[PATCH V3 0/7] remoteproc: imx_rproc: support iMX8MQ/M

[PATCH V3 0/7] remoteproc: imx_rproc: support iMX8MQ/M

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

V3:
 Since I was quite busy in the past days, V3 is late
 Rebased on Linux-next
 Add R-b tags
 1/7: Add R-b tag of Mathieu, add comments
 4/7: Typo fix
 5/7: Add R-b tag of Mathieu, drop index Per Mathieu's comments
 6/7: Add R-b tag of Mathieu
 7/7: Add comment for vqid << 16, drop unneeded timeout settings of mailbox
      Use queue_work instead of schedule_delayed_work
      free mbox channels when remove

V2:
 Rebased on linux-next
 Dropped early boot feature to make patchset simple.
 Drop rsc-da
 https://patchwork.kernel.org/project/linux-remoteproc/cover/20200927064131.24101-1-peng.fan@nxp.com/

V1:
 https://patchwork.kernel.org/cover/11682461/

This patchset is to support i.MX8MQ/M coproc.
The early boot feature was dropped to make the patchset small in V2.

Since i.MX specific TCM memory requirement, add elf platform hook.
Several patches have got reviewed by Oleksij and Mathieu in v1.

Peng Fan (7):
  remoteproc: elf: support platform specific memory hook
  remoteproc: imx_rproc: add elf memory hooks
  remoteproc: imx_rproc: correct err message
  remoteproc: imx_rproc: use devm_ioremap
  remoteproc: imx_rproc: add i.MX specific parse fw hook
  remoteproc: imx_rproc: support i.MX8MQ/M
  remoteproc: imx_proc: enable virtio/mailbox

 drivers/remoteproc/imx_rproc.c             | 290 ++++++++++++++++++++-
 drivers/remoteproc/remoteproc_elf_loader.c |  20 +-
 include/linux/remoteproc.h                 |   4 +
 3 files changed, 306 insertions(+), 8 deletions(-)

-- 
2.28.0

[PATCH V3 1/7] remoteproc: elf: support platform specific memory hook

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

To arm64, "dc      zva, dst" is used in memset.
Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,

"If the memory region being zeroed is any type of Device memory,
this instruction can give an alignment fault which is prioritized
in the same way as other alignment faults that are determined
by the memory type."

On i.MX platforms, when elf is loaded to onchip TCM area, the region
is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
on i.MX not able to write correct data to TCM area.

So we need to use io helpers, and extend the elf loader to support
platform specific memory functions.

Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_elf_loader.c | 20 ++++++++++++++++++--
 include/linux/remoteproc.h                 |  4 ++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index df68d87752e4..6cb71fe47261 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
 }
 EXPORT_SYMBOL(rproc_elf_get_boot_addr);
 
+static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const void *src, size_t count)
+{
+	if (!rproc->ops->elf_memcpy)
+		memcpy(dest, src, count);
+
+	rproc->ops->elf_memcpy(rproc, dest, src, count);
+}
+
+static void rproc_elf_memset(struct rproc *rproc, void *s, int c, size_t count)
+{
+	if (!rproc->ops->elf_memset)
+		memset(s, c, count);
+
+	rproc->ops->elf_memset(rproc, s, c, count);
+}
+
 /**
  * rproc_elf_load_segments() - load firmware segments to memory
  * @rproc: remote processor which will be booted using these fw segments
@@ -214,7 +230,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 
 		/* put the segment where the remote processor expects it */
 		if (filesz)
-			memcpy(ptr, elf_data + offset, filesz);
+			rproc_elf_memcpy(rproc, ptr, elf_data + offset, filesz);
 
 		/*
 		 * Zero out remaining memory for this segment.
@@ -224,7 +240,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 		 * this.
 		 */
 		if (memsz > filesz)
-			memset(ptr + filesz, 0, memsz - filesz);
+			rproc_elf_memset(rproc, ptr + filesz, 0, memsz - filesz);
 	}
 
 	return ret;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e8ac041c64d9..06c52f88a3fd 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -373,6 +373,8 @@ enum rsc_handling_status {
  *			expects to find it
  * @sanity_check:	sanity check the fw image
  * @get_boot_addr:	get boot address to entry point specified in firmware
+ * @elf_memcpy:		platform specific elf loader memcpy
+ * @elf_memset:		platform specific elf loader memset
  * @panic:	optional callback to react to system panic, core will delay
  *		panic at least the returned number of milliseconds
  */
@@ -392,6 +394,8 @@ struct rproc_ops {
 	int (*load)(struct rproc *rproc, const struct firmware *fw);
 	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
 	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
+	void (*elf_memcpy)(struct rproc *rproc, void *dest, const void *src, size_t count);
+	void (*elf_memset)(struct rproc *rproc, void *s, int c, size_t count);
 	unsigned long (*panic)(struct rproc *rproc);
 };
 
-- 
2.28.0

Re: [PATCH V3 1/7] remoteproc: elf: support platform specific memory hook

[Bjorn Andersson]

On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> To arm64, "dc      zva, dst" is used in memset.
> Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> 
> "If the memory region being zeroed is any type of Device memory,
> this instruction can give an alignment fault which is prioritized
> in the same way as other alignment faults that are determined
> by the memory type."
> 
> On i.MX platforms, when elf is loaded to onchip TCM area, the region
> is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
> on i.MX not able to write correct data to TCM area.
> 
> So we need to use io helpers, and extend the elf loader to support
> platform specific memory functions.
> 
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c | 20 ++++++++++++++++++--
>  include/linux/remoteproc.h                 |  4 ++++
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index df68d87752e4..6cb71fe47261 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
>  }
>  EXPORT_SYMBOL(rproc_elf_get_boot_addr);
>  
> +static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const void *src, size_t count)
> +{
> +	if (!rproc->ops->elf_memcpy)
> +		memcpy(dest, src, count);
> +
> +	rproc->ops->elf_memcpy(rproc, dest, src, count);

Looking at the current set of remoteproc drivers I get a feeling that
we'll end up with a while bunch of functions that all just wraps
memcpy_toio(). And the reason for this is that we are we're "abusing" the
carveout to carry the __iomem pointer without keeping track of it.

And this is not the only time we're supposed to use an io-accessor,
another example is rproc_copy_segment() in rproc_coredump.c

It also means that if a platform driver for some reason where to support
both ioremap and normal carveouts the elf_memcpy op would be quite
quirky.


So I would prefer if we track the knowledge about void *va being a
__iomem or not in the struct rproc_mem_entry and make rproc_da_to_va()
return this information as well.

Then instead of extending the ops we can make this simply call memcpy or
memcpy_toio() depending on this.

Regards,
Bjorn

> +}
> +
> +static void rproc_elf_memset(struct rproc *rproc, void *s, int c, size_t count)
> +{
> +	if (!rproc->ops->elf_memset)
> +		memset(s, c, count);
> +
> +	rproc->ops->elf_memset(rproc, s, c, count);
> +}
> +
>  /**
>   * rproc_elf_load_segments() - load firmware segments to memory
>   * @rproc: remote processor which will be booted using these fw segments
> @@ -214,7 +230,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  
>  		/* put the segment where the remote processor expects it */
>  		if (filesz)
> -			memcpy(ptr, elf_data + offset, filesz);
> +			rproc_elf_memcpy(rproc, ptr, elf_data + offset, filesz);
>  
>  		/*
>  		 * Zero out remaining memory for this segment.
> @@ -224,7 +240,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  		 * this.
>  		 */
>  		if (memsz > filesz)
> -			memset(ptr + filesz, 0, memsz - filesz);
> +			rproc_elf_memset(rproc, ptr + filesz, 0, memsz - filesz);
>  	}
>  
>  	return ret;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e8ac041c64d9..06c52f88a3fd 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -373,6 +373,8 @@ enum rsc_handling_status {
>   *			expects to find it
>   * @sanity_check:	sanity check the fw image
>   * @get_boot_addr:	get boot address to entry point specified in firmware
> + * @elf_memcpy:		platform specific elf loader memcpy
> + * @elf_memset:		platform specific elf loader memset
>   * @panic:	optional callback to react to system panic, core will delay
>   *		panic at least the returned number of milliseconds
>   */
> @@ -392,6 +394,8 @@ struct rproc_ops {
>  	int (*load)(struct rproc *rproc, const struct firmware *fw);
>  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
>  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
> +	void (*elf_memcpy)(struct rproc *rproc, void *dest, const void *src, size_t count);
> +	void (*elf_memset)(struct rproc *rproc, void *s, int c, size_t count);
>  	unsigned long (*panic)(struct rproc *rproc);
>  };
>  
> -- 
> 2.28.0
> 

RE: [PATCH V3 1/7] remoteproc: elf: support platform specific memory hook

[Peng Fan]

Hi Bjorn,

> Subject: Re: [PATCH V3 1/7] remoteproc: elf: support platform specific
> memory hook
> 
> On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> 
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > To arm64, "dc      zva, dst" is used in memset.
> > Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> >
> > "If the memory region being zeroed is any type of Device memory, this
> > instruction can give an alignment fault which is prioritized in the
> > same way as other alignment faults that are determined by the memory
> > type."
> >
> > On i.MX platforms, when elf is loaded to onchip TCM area, the region
> > is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
> > on i.MX not able to write correct data to TCM area.
> >
> > So we need to use io helpers, and extend the elf loader to support
> > platform specific memory functions.
> >
> > Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_elf_loader.c | 20
> ++++++++++++++++++--
> >  include/linux/remoteproc.h                 |  4 ++++
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> > b/drivers/remoteproc/remoteproc_elf_loader.c
> > index df68d87752e4..6cb71fe47261 100644
> > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > @@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc *rproc,
> > const struct firmware *fw)  }
> EXPORT_SYMBOL(rproc_elf_get_boot_addr);
> >
> > +static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const
> > +void *src, size_t count) {
> > +	if (!rproc->ops->elf_memcpy)
> > +		memcpy(dest, src, count);
> > +
> > +	rproc->ops->elf_memcpy(rproc, dest, src, count);
> 
> Looking at the current set of remoteproc drivers I get a feeling that we'll end
> up with a while bunch of functions that all just wraps memcpy_toio(). And the
> reason for this is that we are we're "abusing" the carveout to carry the
> __iomem pointer without keeping track of it.
> 
> And this is not the only time we're supposed to use an io-accessor, another
> example is rproc_copy_segment() in rproc_coredump.c
> 
> It also means that if a platform driver for some reason where to support both
> ioremap and normal carveouts the elf_memcpy op would be quite quirky.
> 
> 
> So I would prefer if we track the knowledge about void *va being a __iomem
> or not in the struct rproc_mem_entry and make rproc_da_to_va() return this
> information as well.
> 
> Then instead of extending the ops we can make this simply call memcpy or
> memcpy_toio() depending on this.

A draft proposal as below, are you ok with the approach?

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 46c2937ebea9..bbb6e0613c1b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -189,13 +189,13 @@ EXPORT_SYMBOL(rproc_va_to_pa);
  * here the output of the DMA API for the carveouts, which should be more
  * correct.
  */
-void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
+void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *iomem)
 {
        struct rproc_mem_entry *carveout;
        void *ptr = NULL;

        if (rproc->ops->da_to_va) {
-               ptr = rproc->ops->da_to_va(rproc, da, len);
+               ptr = rproc->ops->da_to_va(rproc, da, len, iomem);
                if (ptr)
                        goto out;
        }
@@ -217,6 +217,9 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)

                ptr = carveout->va + offset;

+               if (iomem)
+                       iomem = carveout->iomem;
+
                break;
        }

diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
index 34530dc20cb4..5ff9389e6319 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -153,18 +153,22 @@ static void rproc_copy_segment(struct rproc *rproc, void *dest,
                               size_t offset, size_t size)
 {
        void *ptr;
+       bool iomem;

        if (segment->dump) {
                segment->dump(rproc, segment, dest, offset, size);
        } else {
-               ptr = rproc_da_to_va(rproc, segment->da + offset, size);
+               ptr = rproc_da_to_va(rproc, segment->da + offset, size, &iomem);
                if (!ptr) {
                        dev_err(&rproc->dev,
                                "invalid copy request for segment %pad with offset %zu and size %zu)\n",
                                &segment->da, offset, size);
                        memset(dest, 0xff, size);
                } else {
-                       memcpy(dest, ptr, size);
+                       if (iomem)
+                               memcpy_fromio(dest, ptr, size);
+                       else
+                               memcpy(dest, ptr, size);
                }
        }
 }
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index df68d87752e4..20538143249e 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -175,6 +175,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                u64 offset = elf_phdr_get_p_offset(class, phdr);
                u32 type = elf_phdr_get_p_type(class, phdr);
                void *ptr;
+               bool iomem;

                if (type != PT_LOAD)
                        continue;
@@ -204,7 +205,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                }

                /* grab the kernel address for this device address */
-               ptr = rproc_da_to_va(rproc, da, memsz);
+               ptr = rproc_da_to_va(rproc, da, memsz, &iomem);
                if (!ptr) {
                        dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
                                memsz);
@@ -213,8 +214,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                }

                /* put the segment where the remote processor expects it */
-               if (filesz)
-                       memcpy(ptr, elf_data + offset, filesz);
+               if (filesz) {
+                       if (iomem)
+                               memcpy_fromio(ptr, elf_data + offset, filesz);
+                       else
+                               memcpy(ptr, elf_data + offset, filesz);
+               }

                /*
                 * Zero out remaining memory for this segment.
@@ -223,8 +228,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                 * did this for us. albeit harmless, we may consider removing
                 * this.
                 */
-               if (memsz > filesz)
-                       memset(ptr + filesz, 0, memsz - filesz);
+               if (memsz > filesz) {
+                       if (iomem)
+                               memset_toio(ptr + filesz, 0, memsz - filesz);
+                       else
+                               memset(ptr + filesz, 0, memsz - filesz);
+               }
        }

        return ret;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e8ac041c64d9..01bb9fa12784 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -329,6 +329,7 @@ struct rproc;
  */
 struct rproc_mem_entry {
        void *va;
+       bool iomem;
        dma_addr_t dma;
        size_t len;
        u32 da;
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index d6473a72a336..dfa0bd7812a5 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -194,7 +194,7 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
 }

 static __always_inline unsigned long __must_check
-copy_to_user(void __user *to, const void *from, unsigned long n)
+copy_to_user(void __user *to, const void *_toiofrom, unsigned long n)
 {
        if (likely(check_copy_size(from, n, true)))
                n = _copy_to_user(to, from, n);

Thanks,
Peng.

> 
> Regards,
> Bjorn
> 
> > +}
> > +
> > +static void rproc_elf_memset(struct rproc *rproc, void *s, int c,
> > +size_t count) {
> > +	if (!rproc->ops->elf_memset)
> > +		memset(s, c, count);
> > +
> > +	rproc->ops->elf_memset(rproc, s, c, count); }
> > +
> >  /**
> >   * rproc_elf_load_segments() - load firmware segments to memory
> >   * @rproc: remote processor which will be booted using these fw
> > segments @@ -214,7 +230,7 @@ int rproc_elf_load_segments(struct rproc
> > *rproc, const struct firmware *fw)
> >
> >  		/* put the segment where the remote processor expects it */
> >  		if (filesz)
> > -			memcpy(ptr, elf_data + offset, filesz);
> > +			rproc_elf_memcpy(rproc, ptr, elf_data + offset, filesz);
> >
> >  		/*
> >  		 * Zero out remaining memory for this segment.
> > @@ -224,7 +240,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
> const struct firmware *fw)
> >  		 * this.
> >  		 */
> >  		if (memsz > filesz)
> > -			memset(ptr + filesz, 0, memsz - filesz);
> > +			rproc_elf_memset(rproc, ptr + filesz, 0, memsz - filesz);
> >  	}
> >
> >  	return ret;
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index e8ac041c64d9..06c52f88a3fd 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -373,6 +373,8 @@ enum rsc_handling_status {
> >   *			expects to find it
> >   * @sanity_check:	sanity check the fw image
> >   * @get_boot_addr:	get boot address to entry point specified in
> firmware
> > + * @elf_memcpy:		platform specific elf loader memcpy
> > + * @elf_memset:		platform specific elf loader memset
> >   * @panic:	optional callback to react to system panic, core will delay
> >   *		panic at least the returned number of milliseconds
> >   */
> > @@ -392,6 +394,8 @@ struct rproc_ops {
> >  	int (*load)(struct rproc *rproc, const struct firmware *fw);
> >  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
> >  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware
> > *fw);
> > +	void (*elf_memcpy)(struct rproc *rproc, void *dest, const void *src,
> size_t count);
> > +	void (*elf_memset)(struct rproc *rproc, void *s, int c, size_t
> > +count);
> >  	unsigned long (*panic)(struct rproc *rproc);  };
> >
> > --
> > 2.28.0
> >

RE: [PATCH V3 1/7] remoteproc: elf: support platform specific memory hook

[Peng Fan]

> Subject: RE: [PATCH V3 1/7] remoteproc: elf: support platform specific
> memory hook
> 
> Hi Bjorn,
> 
> > Subject: Re: [PATCH V3 1/7] remoteproc: elf: support platform specific
> > memory hook
> >
> > On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > To arm64, "dc      zva, dst" is used in memset.
> > > Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> > >
> > > "If the memory region being zeroed is any type of Device memory,
> > > this instruction can give an alignment fault which is prioritized in
> > > the same way as other alignment faults that are determined by the
> > > memory type."
> > >
> > > On i.MX platforms, when elf is loaded to onchip TCM area, the region
> > > is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
> > > on i.MX not able to write correct data to TCM area.
> > >
> > > So we need to use io helpers, and extend the elf loader to support
> > > platform specific memory functions.
> > >
> > > Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > ---
> > >  drivers/remoteproc/remoteproc_elf_loader.c | 20
> > ++++++++++++++++++--
> > >  include/linux/remoteproc.h                 |  4 ++++
> > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> > > b/drivers/remoteproc/remoteproc_elf_loader.c
> > > index df68d87752e4..6cb71fe47261 100644
> > > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > > @@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc
> > > *rproc, const struct firmware *fw)  }
> > EXPORT_SYMBOL(rproc_elf_get_boot_addr);
> > >
> > > +static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const
> > > +void *src, size_t count) {
> > > +	if (!rproc->ops->elf_memcpy)
> > > +		memcpy(dest, src, count);
> > > +
> > > +	rproc->ops->elf_memcpy(rproc, dest, src, count);
> >
> > Looking at the current set of remoteproc drivers I get a feeling that
> > we'll end up with a while bunch of functions that all just wraps
> > memcpy_toio(). And the reason for this is that we are we're "abusing"
> > the carveout to carry the __iomem pointer without keeping track of it.
> >
> > And this is not the only time we're supposed to use an io-accessor,
> > another example is rproc_copy_segment() in rproc_coredump.c
> >
> > It also means that if a platform driver for some reason where to
> > support both ioremap and normal carveouts the elf_memcpy op would be
> quite quirky.
> >
> >
> > So I would prefer if we track the knowledge about void *va being a
> > __iomem or not in the struct rproc_mem_entry and make rproc_da_to_va()
> > return this information as well.
> >
> > Then instead of extending the ops we can make this simply call memcpy
> > or
> > memcpy_toio() depending on this.
> 
> A draft proposal as below, are you ok with the approach?

Mathieu, do you have any comments?

Thanks,
Peng.

> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 46c2937ebea9..bbb6e0613c1b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -189,13 +189,13 @@ EXPORT_SYMBOL(rproc_va_to_pa);
>   * here the output of the DMA API for the carveouts, which should be more
>   * correct.
>   */
> -void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
> +*iomem)
>  {
>         struct rproc_mem_entry *carveout;
>         void *ptr = NULL;
> 
>         if (rproc->ops->da_to_va) {
> -               ptr = rproc->ops->da_to_va(rproc, da, len);
> +               ptr = rproc->ops->da_to_va(rproc, da, len, iomem);
>                 if (ptr)
>                         goto out;
>         }
> @@ -217,6 +217,9 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da,
> size_t len)
> 
>                 ptr = carveout->va + offset;
> 
> +               if (iomem)
> +                       iomem = carveout->iomem;
> +
>                 break;
>         }
> 
> diff --git a/drivers/remoteproc/remoteproc_coredump.c
> b/drivers/remoteproc/remoteproc_coredump.c
> index 34530dc20cb4..5ff9389e6319 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -153,18 +153,22 @@ static void rproc_copy_segment(struct rproc *rproc,
> void *dest,
>                                size_t offset, size_t size)  {
>         void *ptr;
> +       bool iomem;
> 
>         if (segment->dump) {
>                 segment->dump(rproc, segment, dest, offset, size);
>         } else {
> -               ptr = rproc_da_to_va(rproc, segment->da + offset, size);
> +               ptr = rproc_da_to_va(rproc, segment->da + offset, size,
> + &iomem);
>                 if (!ptr) {
>                         dev_err(&rproc->dev,
>                                 "invalid copy request for
> segment %pad with offset %zu and size %zu)\n",
>                                 &segment->da, offset, size);
>                         memset(dest, 0xff, size);
>                 } else {
> -                       memcpy(dest, ptr, size);
> +                       if (iomem)
> +                               memcpy_fromio(dest, ptr, size);
> +                       else
> +                               memcpy(dest, ptr, size);
>                 }
>         }
>  }
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> b/drivers/remoteproc/remoteproc_elf_loader.c
> index df68d87752e4..20538143249e 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -175,6 +175,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
> const struct firmware *fw)
>                 u64 offset = elf_phdr_get_p_offset(class, phdr);
>                 u32 type = elf_phdr_get_p_type(class, phdr);
>                 void *ptr;
> +               bool iomem;
> 
>                 if (type != PT_LOAD)
>                         continue;
> @@ -204,7 +205,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
> const struct firmware *fw)
>                 }
> 
>                 /* grab the kernel address for this device address */
> -               ptr = rproc_da_to_va(rproc, da, memsz);
> +               ptr = rproc_da_to_va(rproc, da, memsz, &iomem);
>                 if (!ptr) {
>                         dev_err(dev, "bad phdr da 0x%llx mem
> 0x%llx\n", da,
>                                 memsz);
> @@ -213,8 +214,12 @@ int rproc_elf_load_segments(struct rproc *rproc,
> const struct firmware *fw)
>                 }
> 
>                 /* put the segment where the remote processor expects
> it */
> -               if (filesz)
> -                       memcpy(ptr, elf_data + offset, filesz);
> +               if (filesz) {
> +                       if (iomem)
> +                               memcpy_fromio(ptr, elf_data + offset,
> filesz);
> +                       else
> +                               memcpy(ptr, elf_data + offset, filesz);
> +               }
> 
>                 /*
>                  * Zero out remaining memory for this segment.
> @@ -223,8 +228,12 @@ int rproc_elf_load_segments(struct rproc *rproc,
> const struct firmware *fw)
>                  * did this for us. albeit harmless, we may consider
> removing
>                  * this.
>                  */
> -               if (memsz > filesz)
> -                       memset(ptr + filesz, 0, memsz - filesz);
> +               if (memsz > filesz) {
> +                       if (iomem)
> +                               memset_toio(ptr + filesz, 0, memsz -
> filesz);
> +                       else
> +                               memset(ptr + filesz, 0, memsz -
> filesz);
> +               }
>         }
> 
>         return ret;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index
> e8ac041c64d9..01bb9fa12784 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -329,6 +329,7 @@ struct rproc;
>   */
>  struct rproc_mem_entry {
>         void *va;
> +       bool iomem;
>         dma_addr_t dma;
>         size_t len;
>         u32 da;
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index
> d6473a72a336..dfa0bd7812a5 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -194,7 +194,7 @@ copy_from_user(void *to, const void __user *from,
> unsigned long n)  }
> 
>  static __always_inline unsigned long __must_check -copy_to_user(void
> __user *to, const void *from, unsigned long n)
> +copy_to_user(void __user *to, const void *_toiofrom, unsigned long n)
>  {
>         if (likely(check_copy_size(from, n, true)))
>                 n = _copy_to_user(to, from, n);
> 
> Thanks,
> Peng.
> 
> >
> > Regards,
> > Bjorn
> >
> > > +}
> > > +
> > > +static void rproc_elf_memset(struct rproc *rproc, void *s, int c,
> > > +size_t count) {
> > > +	if (!rproc->ops->elf_memset)
> > > +		memset(s, c, count);
> > > +
> > > +	rproc->ops->elf_memset(rproc, s, c, count); }
> > > +
> > >  /**
> > >   * rproc_elf_load_segments() - load firmware segments to memory
> > >   * @rproc: remote processor which will be booted using these fw
> > > segments @@ -214,7 +230,7 @@ int rproc_elf_load_segments(struct
> > > rproc *rproc, const struct firmware *fw)
> > >
> > >  		/* put the segment where the remote processor expects it */
> > >  		if (filesz)
> > > -			memcpy(ptr, elf_data + offset, filesz);
> > > +			rproc_elf_memcpy(rproc, ptr, elf_data + offset, filesz);
> > >
> > >  		/*
> > >  		 * Zero out remaining memory for this segment.
> > > @@ -224,7 +240,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
> > const struct firmware *fw)
> > >  		 * this.
> > >  		 */
> > >  		if (memsz > filesz)
> > > -			memset(ptr + filesz, 0, memsz - filesz);
> > > +			rproc_elf_memset(rproc, ptr + filesz, 0, memsz - filesz);
> > >  	}
> > >
> > >  	return ret;
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index e8ac041c64d9..06c52f88a3fd 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -373,6 +373,8 @@ enum rsc_handling_status {
> > >   *			expects to find it
> > >   * @sanity_check:	sanity check the fw image
> > >   * @get_boot_addr:	get boot address to entry point specified in
> > firmware
> > > + * @elf_memcpy:		platform specific elf loader memcpy
> > > + * @elf_memset:		platform specific elf loader memset
> > >   * @panic:	optional callback to react to system panic, core will delay
> > >   *		panic at least the returned number of milliseconds
> > >   */
> > > @@ -392,6 +394,8 @@ struct rproc_ops {
> > >  	int (*load)(struct rproc *rproc, const struct firmware *fw);
> > >  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
> > >  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware
> > > *fw);
> > > +	void (*elf_memcpy)(struct rproc *rproc, void *dest, const void
> > > +*src,
> > size_t count);
> > > +	void (*elf_memset)(struct rproc *rproc, void *s, int c, size_t
> > > +count);
> > >  	unsigned long (*panic)(struct rproc *rproc);  };
> > >
> > > --
> > > 2.28.0
> > >

Re: [PATCH V3 1/7] remoteproc: elf: support platform specific memory hook

[Mathieu Poirier]

On Wed, 9 Dec 2020 at 08:00, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: RE: [PATCH V3 1/7] remoteproc: elf: support platform specific
> > memory hook
> >
> > Hi Bjorn,
> >
> > > Subject: Re: [PATCH V3 1/7] remoteproc: elf: support platform specific
> > > memory hook
> > >
> > > On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > To arm64, "dc      zva, dst" is used in memset.
> > > > Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> > > >
> > > > "If the memory region being zeroed is any type of Device memory,
> > > > this instruction can give an alignment fault which is prioritized in
> > > > the same way as other alignment faults that are determined by the
> > > > memory type."
> > > >
> > > > On i.MX platforms, when elf is loaded to onchip TCM area, the region
> > > > is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
> > > > on i.MX not able to write correct data to TCM area.
> > > >
> > > > So we need to use io helpers, and extend the elf loader to support
> > > > platform specific memory functions.
> > > >
> > > > Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > ---
> > > >  drivers/remoteproc/remoteproc_elf_loader.c | 20
> > > ++++++++++++++++++--
> > > >  include/linux/remoteproc.h                 |  4 ++++
> > > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> > > > b/drivers/remoteproc/remoteproc_elf_loader.c
> > > > index df68d87752e4..6cb71fe47261 100644
> > > > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > > > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > > > @@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc
> > > > *rproc, const struct firmware *fw)  }
> > > EXPORT_SYMBOL(rproc_elf_get_boot_addr);
> > > >
> > > > +static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const
> > > > +void *src, size_t count) {
> > > > + if (!rproc->ops->elf_memcpy)
> > > > +         memcpy(dest, src, count);
> > > > +
> > > > + rproc->ops->elf_memcpy(rproc, dest, src, count);
> > >
> > > Looking at the current set of remoteproc drivers I get a feeling that
> > > we'll end up with a while bunch of functions that all just wraps
> > > memcpy_toio(). And the reason for this is that we are we're "abusing"
> > > the carveout to carry the __iomem pointer without keeping track of it.
> > >
> > > And this is not the only time we're supposed to use an io-accessor,
> > > another example is rproc_copy_segment() in rproc_coredump.c
> > >
> > > It also means that if a platform driver for some reason where to
> > > support both ioremap and normal carveouts the elf_memcpy op would be
> > quite quirky.
> > >
> > >
> > > So I would prefer if we track the knowledge about void *va being a
> > > __iomem or not in the struct rproc_mem_entry and make rproc_da_to_va()
> > > return this information as well.
> > >
> > > Then instead of extending the ops we can make this simply call memcpy
> > > or
> > > memcpy_toio() depending on this.
> >
> > A draft proposal as below, are you ok with the approach?
>
> Mathieu, do you have any comments?
>

I will look into this on Monday.

Re: [PATCH V3 1/7] remoteproc: elf: support platform specific memory hook

[Bjorn Andersson]

On Sun 06 Dec 20:07 CST 2020, Peng Fan wrote:

> Hi Bjorn,
> 
> > Subject: Re: [PATCH V3 1/7] remoteproc: elf: support platform specific
> > memory hook
> > 
> > On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> > 
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > To arm64, "dc      zva, dst" is used in memset.
> > > Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> > >
> > > "If the memory region being zeroed is any type of Device memory, this
> > > instruction can give an alignment fault which is prioritized in the
> > > same way as other alignment faults that are determined by the memory
> > > type."
> > >
> > > On i.MX platforms, when elf is loaded to onchip TCM area, the region
> > > is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
> > > on i.MX not able to write correct data to TCM area.
> > >
> > > So we need to use io helpers, and extend the elf loader to support
> > > platform specific memory functions.
> > >
> > > Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > ---
> > >  drivers/remoteproc/remoteproc_elf_loader.c | 20
> > ++++++++++++++++++--
> > >  include/linux/remoteproc.h                 |  4 ++++
> > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> > > b/drivers/remoteproc/remoteproc_elf_loader.c
> > > index df68d87752e4..6cb71fe47261 100644
> > > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > > @@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc *rproc,
> > > const struct firmware *fw)  }
> > EXPORT_SYMBOL(rproc_elf_get_boot_addr);
> > >
> > > +static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const
> > > +void *src, size_t count) {
> > > +	if (!rproc->ops->elf_memcpy)
> > > +		memcpy(dest, src, count);
> > > +
> > > +	rproc->ops->elf_memcpy(rproc, dest, src, count);
> > 
> > Looking at the current set of remoteproc drivers I get a feeling that we'll end
> > up with a while bunch of functions that all just wraps memcpy_toio(). And the
> > reason for this is that we are we're "abusing" the carveout to carry the
> > __iomem pointer without keeping track of it.
> > 
> > And this is not the only time we're supposed to use an io-accessor, another
> > example is rproc_copy_segment() in rproc_coredump.c
> > 
> > It also means that if a platform driver for some reason where to support both
> > ioremap and normal carveouts the elf_memcpy op would be quite quirky.
> > 
> > 
> > So I would prefer if we track the knowledge about void *va being a __iomem
> > or not in the struct rproc_mem_entry and make rproc_da_to_va() return this
> > information as well.
> > 
> > Then instead of extending the ops we can make this simply call memcpy or
> > memcpy_toio() depending on this.
> 
> A draft proposal as below, are you ok with the approach?
> 

Yes, this looks very reasonable and should be directly useful for the
other users of ioremap as well.

May I ask that you rename the boolean "iomem" with "is_iomem", to make
it obvious that it's a boolean in the cases where we're already juggling
physical, virtual and device addresses?

Thanks,
Bjorn

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 46c2937ebea9..bbb6e0613c1b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -189,13 +189,13 @@ EXPORT_SYMBOL(rproc_va_to_pa);
>   * here the output of the DMA API for the carveouts, which should be more
>   * correct.
>   */
> -void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *iomem)
>  {
>         struct rproc_mem_entry *carveout;
>         void *ptr = NULL;
> 
>         if (rproc->ops->da_to_va) {
> -               ptr = rproc->ops->da_to_va(rproc, da, len);
> +               ptr = rproc->ops->da_to_va(rproc, da, len, iomem);
>                 if (ptr)
>                         goto out;
>         }
> @@ -217,6 +217,9 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> 
>                 ptr = carveout->va + offset;
> 
> +               if (iomem)
> +                       iomem = carveout->iomem;
> +
>                 break;
>         }
> 
> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
> index 34530dc20cb4..5ff9389e6319 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -153,18 +153,22 @@ static void rproc_copy_segment(struct rproc *rproc, void *dest,
>                                size_t offset, size_t size)
>  {
>         void *ptr;
> +       bool iomem;
> 
>         if (segment->dump) {
>                 segment->dump(rproc, segment, dest, offset, size);
>         } else {
> -               ptr = rproc_da_to_va(rproc, segment->da + offset, size);
> +               ptr = rproc_da_to_va(rproc, segment->da + offset, size, &iomem);
>                 if (!ptr) {
>                         dev_err(&rproc->dev,
>                                 "invalid copy request for segment %pad with offset %zu and size %zu)\n",
>                                 &segment->da, offset, size);
>                         memset(dest, 0xff, size);
>                 } else {
> -                       memcpy(dest, ptr, size);
> +                       if (iomem)
> +                               memcpy_fromio(dest, ptr, size);
> +                       else
> +                               memcpy(dest, ptr, size);
>                 }
>         }
>  }
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index df68d87752e4..20538143249e 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -175,6 +175,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>                 u64 offset = elf_phdr_get_p_offset(class, phdr);
>                 u32 type = elf_phdr_get_p_type(class, phdr);
>                 void *ptr;
> +               bool iomem;
> 
>                 if (type != PT_LOAD)
>                         continue;
> @@ -204,7 +205,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>                 }
> 
>                 /* grab the kernel address for this device address */
> -               ptr = rproc_da_to_va(rproc, da, memsz);
> +               ptr = rproc_da_to_va(rproc, da, memsz, &iomem);
>                 if (!ptr) {
>                         dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
>                                 memsz);
> @@ -213,8 +214,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>                 }
> 
>                 /* put the segment where the remote processor expects it */
> -               if (filesz)
> -                       memcpy(ptr, elf_data + offset, filesz);
> +               if (filesz) {
> +                       if (iomem)
> +                               memcpy_fromio(ptr, elf_data + offset, filesz);
> +                       else
> +                               memcpy(ptr, elf_data + offset, filesz);
> +               }
> 
>                 /*
>                  * Zero out remaining memory for this segment.
> @@ -223,8 +228,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>                  * did this for us. albeit harmless, we may consider removing
>                  * this.
>                  */
> -               if (memsz > filesz)
> -                       memset(ptr + filesz, 0, memsz - filesz);
> +               if (memsz > filesz) {
> +                       if (iomem)
> +                               memset_toio(ptr + filesz, 0, memsz - filesz);
> +                       else
> +                               memset(ptr + filesz, 0, memsz - filesz);
> +               }
>         }
> 
>         return ret;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e8ac041c64d9..01bb9fa12784 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -329,6 +329,7 @@ struct rproc;
>   */
>  struct rproc_mem_entry {
>         void *va;
> +       bool iomem;
>         dma_addr_t dma;
>         size_t len;
>         u32 da;
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index d6473a72a336..dfa0bd7812a5 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -194,7 +194,7 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
>  }
> 
>  static __always_inline unsigned long __must_check
> -copy_to_user(void __user *to, const void *from, unsigned long n)
> +copy_to_user(void __user *to, const void *_toiofrom, unsigned long n)
>  {
>         if (likely(check_copy_size(from, n, true)))
>                 n = _copy_to_user(to, from, n);
> 
> Thanks,
> Peng.
> 
> > 
> > Regards,
> > Bjorn
> > 
> > > +}
> > > +
> > > +static void rproc_elf_memset(struct rproc *rproc, void *s, int c,
> > > +size_t count) {
> > > +	if (!rproc->ops->elf_memset)
> > > +		memset(s, c, count);
> > > +
> > > +	rproc->ops->elf_memset(rproc, s, c, count); }
> > > +
> > >  /**
> > >   * rproc_elf_load_segments() - load firmware segments to memory
> > >   * @rproc: remote processor which will be booted using these fw
> > > segments @@ -214,7 +230,7 @@ int rproc_elf_load_segments(struct rproc
> > > *rproc, const struct firmware *fw)
> > >
> > >  		/* put the segment where the remote processor expects it */
> > >  		if (filesz)
> > > -			memcpy(ptr, elf_data + offset, filesz);
> > > +			rproc_elf_memcpy(rproc, ptr, elf_data + offset, filesz);
> > >
> > >  		/*
> > >  		 * Zero out remaining memory for this segment.
> > > @@ -224,7 +240,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
> > const struct firmware *fw)
> > >  		 * this.
> > >  		 */
> > >  		if (memsz > filesz)
> > > -			memset(ptr + filesz, 0, memsz - filesz);
> > > +			rproc_elf_memset(rproc, ptr + filesz, 0, memsz - filesz);
> > >  	}
> > >
> > >  	return ret;
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index e8ac041c64d9..06c52f88a3fd 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -373,6 +373,8 @@ enum rsc_handling_status {
> > >   *			expects to find it
> > >   * @sanity_check:	sanity check the fw image
> > >   * @get_boot_addr:	get boot address to entry point specified in
> > firmware
> > > + * @elf_memcpy:		platform specific elf loader memcpy
> > > + * @elf_memset:		platform specific elf loader memset
> > >   * @panic:	optional callback to react to system panic, core will delay
> > >   *		panic at least the returned number of milliseconds
> > >   */
> > > @@ -392,6 +394,8 @@ struct rproc_ops {
> > >  	int (*load)(struct rproc *rproc, const struct firmware *fw);
> > >  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
> > >  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware
> > > *fw);
> > > +	void (*elf_memcpy)(struct rproc *rproc, void *dest, const void *src,
> > size_t count);
> > > +	void (*elf_memset)(struct rproc *rproc, void *s, int c, size_t
> > > +count);
> > >  	unsigned long (*panic)(struct rproc *rproc);  };
> > >
> > > --
> > > 2.28.0
> > >

Re: [PATCH V3 1/7] remoteproc: elf: support platform specific memory hook

[Mathieu Poirier]

On Thu, Dec 10, 2020 at 10:54:17AM -0600, Bjorn Andersson wrote:
> On Sun 06 Dec 20:07 CST 2020, Peng Fan wrote:
> 
> > Hi Bjorn,
> > 
> > > Subject: Re: [PATCH V3 1/7] remoteproc: elf: support platform specific
> > > memory hook
> > > 
> > > On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> > > 
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > To arm64, "dc      zva, dst" is used in memset.
> > > > Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> > > >
> > > > "If the memory region being zeroed is any type of Device memory, this
> > > > instruction can give an alignment fault which is prioritized in the
> > > > same way as other alignment faults that are determined by the memory
> > > > type."
> > > >
> > > > On i.MX platforms, when elf is loaded to onchip TCM area, the region
> > > > is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
> > > > on i.MX not able to write correct data to TCM area.
> > > >
> > > > So we need to use io helpers, and extend the elf loader to support
> > > > platform specific memory functions.
> > > >
> > > > Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > ---
> > > >  drivers/remoteproc/remoteproc_elf_loader.c | 20
> > > ++++++++++++++++++--
> > > >  include/linux/remoteproc.h                 |  4 ++++
> > > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> > > > b/drivers/remoteproc/remoteproc_elf_loader.c
> > > > index df68d87752e4..6cb71fe47261 100644
> > > > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > > > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > > > @@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc *rproc,
> > > > const struct firmware *fw)  }
> > > EXPORT_SYMBOL(rproc_elf_get_boot_addr);
> > > >
> > > > +static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const
> > > > +void *src, size_t count) {
> > > > +	if (!rproc->ops->elf_memcpy)
> > > > +		memcpy(dest, src, count);
> > > > +
> > > > +	rproc->ops->elf_memcpy(rproc, dest, src, count);
> > > 
> > > Looking at the current set of remoteproc drivers I get a feeling that we'll end
> > > up with a while bunch of functions that all just wraps memcpy_toio(). And the
> > > reason for this is that we are we're "abusing" the carveout to carry the
> > > __iomem pointer without keeping track of it.
> > > 
> > > And this is not the only time we're supposed to use an io-accessor, another
> > > example is rproc_copy_segment() in rproc_coredump.c
> > > 
> > > It also means that if a platform driver for some reason where to support both
> > > ioremap and normal carveouts the elf_memcpy op would be quite quirky.
> > > 
> > > 
> > > So I would prefer if we track the knowledge about void *va being a __iomem
> > > or not in the struct rproc_mem_entry and make rproc_da_to_va() return this
> > > information as well.
> > > 
> > > Then instead of extending the ops we can make this simply call memcpy or
> > > memcpy_toio() depending on this.
> > 
> > A draft proposal as below, are you ok with the approach?
> > 
> 
> Yes, this looks very reasonable and should be directly useful for the
> other users of ioremap as well.

Bjorn is correct in his assessment that using rproc_ops::elf_memcpy and
rproc_ops::elf_memset will lead to platform driver just wrapping xyz_toio().
On the flip side the advantage is that the core code stays the same when the
next wave of memory accessor is needed.

But there is no telling when that will happen and what the requirements will be
so I'm also in favour of moving forward with what you suggested below.  Please
read on, there's a couple of comments to a address.

> 
> May I ask that you rename the boolean "iomem" with "is_iomem", to make
> it obvious that it's a boolean in the cases where we're already juggling
> physical, virtual and device addresses?
> 
> Thanks,
> Bjorn
> 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 46c2937ebea9..bbb6e0613c1b 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -189,13 +189,13 @@ EXPORT_SYMBOL(rproc_va_to_pa);
> >   * here the output of the DMA API for the carveouts, which should be more
> >   * correct.
> >   */
> > -void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> > +void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *iomem)
> >  {
> >         struct rproc_mem_entry *carveout;
> >         void *ptr = NULL;
> > 
> >         if (rproc->ops->da_to_va) {
> > -               ptr = rproc->ops->da_to_va(rproc, da, len);
> > +               ptr = rproc->ops->da_to_va(rproc, da, len, iomem);
> >                 if (ptr)
> >                         goto out;
> >         }
> > @@ -217,6 +217,9 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> > 
> >                 ptr = carveout->va + offset;
> > 
> > +               if (iomem)
> > +                       iomem = carveout->iomem;
> > +
> >                 break;
> >         }
> > 
> > diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
> > index 34530dc20cb4..5ff9389e6319 100644
> > --- a/drivers/remoteproc/remoteproc_coredump.c
> > +++ b/drivers/remoteproc/remoteproc_coredump.c
> > @@ -153,18 +153,22 @@ static void rproc_copy_segment(struct rproc *rproc, void *dest,
> >                                size_t offset, size_t size)
> >  {
> >         void *ptr;
> > +       bool iomem;
> > 
> >         if (segment->dump) {
> >                 segment->dump(rproc, segment, dest, offset, size);
> >         } else {
> > -               ptr = rproc_da_to_va(rproc, segment->da + offset, size);
> > +               ptr = rproc_da_to_va(rproc, segment->da + offset, size, &iomem);
> >                 if (!ptr) {
> >                         dev_err(&rproc->dev,
> >                                 "invalid copy request for segment %pad with offset %zu and size %zu)\n",
> >                                 &segment->da, offset, size);
> >                         memset(dest, 0xff, size);
> >                 } else {
> > -                       memcpy(dest, ptr, size);
> > +                       if (iomem)
> > +                               memcpy_fromio(dest, ptr, size);
> > +                       else
> > +                               memcpy(dest, ptr, size);
> >                 }
> >         }
> >  }
> > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> > index df68d87752e4..20538143249e 100644
> > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > @@ -175,6 +175,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
> >                 u64 offset = elf_phdr_get_p_offset(class, phdr);
> >                 u32 type = elf_phdr_get_p_type(class, phdr);
> >                 void *ptr;
> > +               bool iomem;
> > 
> >                 if (type != PT_LOAD)
> >                         continue;
> > @@ -204,7 +205,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
> >                 }
> > 
> >                 /* grab the kernel address for this device address */
> > -               ptr = rproc_da_to_va(rproc, da, memsz);
> > +               ptr = rproc_da_to_va(rproc, da, memsz, &iomem);
> >                 if (!ptr) {
> >                         dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> >                                 memsz);
> > @@ -213,8 +214,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
> >                 }
> > 
> >                 /* put the segment where the remote processor expects it */
> > -               if (filesz)
> > -                       memcpy(ptr, elf_data + offset, filesz);
> > +               if (filesz) {
> > +                       if (iomem)
> > +                               memcpy_fromio(ptr, elf_data + offset, filesz);
> > +                       else
> > +                               memcpy(ptr, elf_data + offset, filesz);
> > +               }
> > 
> >                 /*
> >                  * Zero out remaining memory for this segment.
> > @@ -223,8 +228,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
> >                  * did this for us. albeit harmless, we may consider removing
> >                  * this.
> >                  */
> > -               if (memsz > filesz)
> > -                       memset(ptr + filesz, 0, memsz - filesz);
> > +               if (memsz > filesz) {
> > +                       if (iomem)
> > +                               memset_toio(ptr + filesz, 0, memsz - filesz);
> > +                       else
> > +                               memset(ptr + filesz, 0, memsz - filesz);
> > +               }
> >         }
> > 
> >         return ret;
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index e8ac041c64d9..01bb9fa12784 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -329,6 +329,7 @@ struct rproc;
> >   */
> >  struct rproc_mem_entry {
> >         void *va;
> > +       bool iomem;

Don't forget to document the new field.

> >         dma_addr_t dma;
> >         size_t len;
> >         u32 da;
> > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> > index d6473a72a336..dfa0bd7812a5 100644
> > --- a/include/linux/uaccess.h
> > +++ b/include/linux/uaccess.h
> > @@ -194,7 +194,7 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
> >  }
> > 
> >  static __always_inline unsigned long __must_check
> > -copy_to_user(void __user *to, const void *from, unsigned long n)
> > +copy_to_user(void __user *to, const void *_toiofrom, unsigned long n)

Why is this needed?

Thanks,
Mathieu

> >  {
> >         if (likely(check_copy_size(from, n, true)))
> >                 n = _copy_to_user(to, from, n);
> > 
> > Thanks,
> > Peng.
> > 
> > > 
> > > Regards,
> > > Bjorn
> > > 
> > > > +}
> > > > +
> > > > +static void rproc_elf_memset(struct rproc *rproc, void *s, int c,
> > > > +size_t count) {
> > > > +	if (!rproc->ops->elf_memset)
> > > > +		memset(s, c, count);
> > > > +
> > > > +	rproc->ops->elf_memset(rproc, s, c, count); }
> > > > +
> > > >  /**
> > > >   * rproc_elf_load_segments() - load firmware segments to memory
> > > >   * @rproc: remote processor which will be booted using these fw
> > > > segments @@ -214,7 +230,7 @@ int rproc_elf_load_segments(struct rproc
> > > > *rproc, const struct firmware *fw)
> > > >
> > > >  		/* put the segment where the remote processor expects it */
> > > >  		if (filesz)
> > > > -			memcpy(ptr, elf_data + offset, filesz);
> > > > +			rproc_elf_memcpy(rproc, ptr, elf_data + offset, filesz);
> > > >
> > > >  		/*
> > > >  		 * Zero out remaining memory for this segment.
> > > > @@ -224,7 +240,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
> > > const struct firmware *fw)
> > > >  		 * this.
> > > >  		 */
> > > >  		if (memsz > filesz)
> > > > -			memset(ptr + filesz, 0, memsz - filesz);
> > > > +			rproc_elf_memset(rproc, ptr + filesz, 0, memsz - filesz);
> > > >  	}
> > > >
> > > >  	return ret;
> > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > > index e8ac041c64d9..06c52f88a3fd 100644
> > > > --- a/include/linux/remoteproc.h
> > > > +++ b/include/linux/remoteproc.h
> > > > @@ -373,6 +373,8 @@ enum rsc_handling_status {
> > > >   *			expects to find it
> > > >   * @sanity_check:	sanity check the fw image
> > > >   * @get_boot_addr:	get boot address to entry point specified in
> > > firmware
> > > > + * @elf_memcpy:		platform specific elf loader memcpy
> > > > + * @elf_memset:		platform specific elf loader memset
> > > >   * @panic:	optional callback to react to system panic, core will delay
> > > >   *		panic at least the returned number of milliseconds
> > > >   */
> > > > @@ -392,6 +394,8 @@ struct rproc_ops {
> > > >  	int (*load)(struct rproc *rproc, const struct firmware *fw);
> > > >  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
> > > >  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware
> > > > *fw);
> > > > +	void (*elf_memcpy)(struct rproc *rproc, void *dest, const void *src,
> > > size_t count);
> > > > +	void (*elf_memset)(struct rproc *rproc, void *s, int c, size_t
> > > > +count);
> > > >  	unsigned long (*panic)(struct rproc *rproc);  };
> > > >
> > > > --
> > > > 2.28.0
> > > >

[PATCH V3 2/7] remoteproc: imx_rproc: add elf memory hooks

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Add elf memory hooks according to elf_mem_hook setting in the platform
configuration dcfg.

Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 8957ed271d20..d1abb253b499 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -6,6 +6,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
@@ -76,6 +77,7 @@ struct imx_rproc_dcfg {
 	u32				src_stop;
 	const struct imx_rproc_att	*att;
 	size_t				att_size;
+	bool				elf_mem_hook;
 };
 
 struct imx_rproc {
@@ -310,6 +312,16 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 	return 0;
 }
 
+static void imx_rproc_memcpy(struct rproc *rproc, void *dest, const void *src, size_t count)
+{
+	memcpy_toio((void * __iomem)dest, src, count);
+}
+
+static void imx_rproc_memset(struct rproc *rproc, void *s, int c, size_t count)
+{
+	memset_io((void * __iomem)s, c, count);
+}
+
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -340,6 +352,11 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_rproc;
 	}
 
+	if (dcfg->elf_mem_hook) {
+		rproc->ops->elf_memcpy = imx_rproc_memcpy;
+		rproc->ops->elf_memset = imx_rproc_memset;
+	}
+
 	priv = rproc->priv;
 	priv->rproc = rproc;
 	priv->regmap = regmap;
-- 
2.28.0

[PATCH V3 3/7] remoteproc: imx_rproc: correct err message

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

It is using devm_ioremap, so not devm_ioremap_resource. Correct
the error message and print out sa/size.

Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index d1abb253b499..aa5fbd0c7768 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -270,7 +270,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
 						     att->sa, att->size);
 		if (!priv->mem[b].cpu_addr) {
-			dev_err(dev, "devm_ioremap_resource failed\n");
+			dev_err(dev, "devm_ioremap failed\n");
 			return -ENOMEM;
 		}
 		priv->mem[b].sys_addr = att->sa;
-- 
2.28.0

Re: [PATCH V3 3/7] remoteproc: imx_rproc: correct err message

[Bjorn Andersson]

On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> It is using devm_ioremap, so not devm_ioremap_resource. Correct
> the error message and print out sa/size.
> 
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index d1abb253b499..aa5fbd0c7768 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -270,7 +270,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
>  						     att->sa, att->size);
>  		if (!priv->mem[b].cpu_addr) {
> -			dev_err(dev, "devm_ioremap_resource failed\n");
> +			dev_err(dev, "devm_ioremap failed\n");

I would prefer if this was expanded to a proper sentence, and I really
like the second part of the commit message to be included in the change.
So something like:

			dev_err(dev, "failed to remap %#x bytes from %#x\n",
				att->size, att->sa);


And similarly below if mapping a memory-region fails.

Thanks,
Bjorn

>  			return -ENOMEM;
>  		}
>  		priv->mem[b].sys_addr = att->sa;
> -- 
> 2.28.0
> 

RE: [PATCH V3 3/7] remoteproc: imx_rproc: correct err message

[Peng Fan]

> Subject: Re: [PATCH V3 3/7] remoteproc: imx_rproc: correct err message
> 
> On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> 
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > It is using devm_ioremap, so not devm_ioremap_resource. Correct the
> > error message and print out sa/size.
> >
> > Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index d1abb253b499..aa5fbd0c7768
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -270,7 +270,7 @@ static int imx_rproc_addr_init(struct imx_rproc
> *priv,
> >  		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
> >  						     att->sa, att->size);
> >  		if (!priv->mem[b].cpu_addr) {
> > -			dev_err(dev, "devm_ioremap_resource failed\n");
> > +			dev_err(dev, "devm_ioremap failed\n");
> 
> I would prefer if this was expanded to a proper sentence, and I really like the
> second part of the commit message to be included in the change.
> So something like:
> 
> 			dev_err(dev, "failed to remap %#x bytes from %#x\n",
> 				att->size, att->sa);
> 
> 
> And similarly below if mapping a memory-region fails.

ok, fix in v4.

Thanks,
Peng.

> 
> Thanks,
> Bjorn
> 
> >  			return -ENOMEM;
> >  		}
> >  		priv->mem[b].sys_addr = att->sa;
> > --
> > 2.28.0
> >

[PATCH V3 4/7] remoteproc: imx_rproc: use devm_ioremap

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

We might need to map an region multiple times, becaue the region might
be shared between remote processors, such i.MX8QM with dual M4 cores.
So use devm_ioremap, not devm_ioremap_resource.

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index aa5fbd0c7768..15c7baa480d7 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -298,9 +298,10 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 		if (b >= IMX7D_RPROC_MEM_MAX)
 			break;
 
-		priv->mem[b].cpu_addr = devm_ioremap_resource(&pdev->dev, &res);
+		/* Not use resource version, because we might share region */
+		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
 		if (IS_ERR(priv->mem[b].cpu_addr)) {
-			dev_err(dev, "devm_ioremap_resource failed\n");
+			dev_err(dev, "devm_ioremap %pR failed\n", &res);
 			err = PTR_ERR(priv->mem[b].cpu_addr);
 			return err;
 		}
-- 
2.28.0

[PATCH V3 5/7] remoteproc: imx_rproc: add i.MX specific parse fw hook

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The hook is used to parse memory-regions and load resource table
from the address the remote processor published.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/imx_rproc.c | 93 ++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 15c7baa480d7..de44b7d6ae63 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -11,6 +11,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -243,10 +244,102 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
 	return va;
 }
 
+static int imx_rproc_mem_alloc(struct rproc *rproc,
+			       struct rproc_mem_entry *mem)
+{
+	struct device *dev = rproc->dev.parent;
+	void *va;
+
+	dev_dbg(dev, "map memory: %p+%zx\n", &mem->dma, mem->len);
+	va = ioremap_wc(mem->dma, mem->len);
+	if (IS_ERR_OR_NULL(va)) {
+		dev_err(dev, "Unable to map memory region: %p+%zx\n",
+			&mem->dma, mem->len);
+		return -ENOMEM;
+	}
+
+	/* Update memory entry va */
+	mem->va = va;
+
+	return 0;
+}
+
+static int imx_rproc_mem_release(struct rproc *rproc,
+				 struct rproc_mem_entry *mem)
+{
+	dev_dbg(rproc->dev.parent, "unmap memory: %pa\n", &mem->dma);
+	iounmap(mem->va);
+
+	return 0;
+}
+
+static int imx_rproc_parse_memory_regions(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	struct device_node *np = priv->dev->of_node;
+	struct of_phandle_iterator it;
+	struct rproc_mem_entry *mem;
+	struct reserved_mem *rmem;
+	u32 da;
+
+	/* Register associated reserved memory regions */
+	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
+	while (of_phandle_iterator_next(&it) == 0) {
+		/*
+		 * Ignore the first memory region which will be used vdev buffer.
+		 * No need to do extra handlings, rproc_add_virtio_dev will handle it.
+		 */
+		if (!strcmp(it.node->name, "vdevbuffer"))
+			continue;
+
+		rmem = of_reserved_mem_lookup(it.node);
+		if (!rmem) {
+			dev_err(priv->dev, "unable to acquire memory-region\n");
+			return -EINVAL;
+		}
+
+		/* No need to translate pa to da, i.MX use same map */
+		da = rmem->base;
+
+		/* Register memory region */
+		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)rmem->base, rmem->size, da,
+					   imx_rproc_mem_alloc, imx_rproc_mem_release,
+					   it.node->name);
+
+		if (mem)
+			rproc_coredump_add_segment(rproc, da, rmem->size);
+		else
+			return -ENOMEM;
+
+		rproc_add_carveout(rproc, mem);
+	}
+
+	return  0;
+}
+
+static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+	int ret = imx_rproc_parse_memory_regions(rproc);
+
+	if (ret)
+		return ret;
+
+	ret = rproc_elf_load_rsc_table(rproc, fw);
+	if (ret)
+		dev_info(&rproc->dev, "No resource table in elf\n");
+
+	return 0;
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
 	.da_to_va       = imx_rproc_da_to_va,
+	.load		= rproc_elf_load_segments,
+	.parse_fw	= imx_rproc_parse_fw,
+	.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 imx_rproc_addr_init(struct imx_rproc *priv,
-- 
2.28.0

[PATCH V3 6/7] remoteproc: imx_rproc: support i.MX8MQ/M

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Add i.MX8MQ dev/sys addr map and configuration data structure
i.MX8MM share i.MX8MQ settings.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/imx_rproc.c | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index de44b7d6ae63..afa650610996 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -90,6 +90,34 @@ struct imx_rproc {
 	struct clk			*clk;
 };
 
+static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
+	/* dev addr , sys addr  , size	    , flags */
+	/* TCML - alias */
+	{ 0x00000000, 0x007e0000, 0x00020000, 0 },
+	/* OCRAM_S */
+	{ 0x00180000, 0x00180000, 0x00008000, 0 },
+	/* OCRAM */
+	{ 0x00900000, 0x00900000, 0x00020000, 0 },
+	/* OCRAM */
+	{ 0x00920000, 0x00920000, 0x00020000, 0 },
+	/* QSPI Code - alias */
+	{ 0x08000000, 0x08000000, 0x08000000, 0 },
+	/* DDR (Code) - alias */
+	{ 0x10000000, 0x80000000, 0x0FFE0000, 0 },
+	/* TCML */
+	{ 0x1FFE0000, 0x007E0000, 0x00020000, ATT_OWN },
+	/* TCMU */
+	{ 0x20000000, 0x00800000, 0x00020000, ATT_OWN },
+	/* OCRAM_S */
+	{ 0x20180000, 0x00180000, 0x00008000, ATT_OWN },
+	/* OCRAM */
+	{ 0x20200000, 0x00900000, 0x00020000, ATT_OWN },
+	/* OCRAM */
+	{ 0x20220000, 0x00920000, 0x00020000, ATT_OWN },
+	/* DDR (Data) */
+	{ 0x40000000, 0x40000000, 0x80000000, 0 },
+};
+
 static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
 	/* dev addr , sys addr  , size	    , flags */
 	/* OCRAM_S (M4 Boot code) - alias */
@@ -140,6 +168,16 @@ static const struct imx_rproc_att imx_rproc_att_imx6sx[] = {
 	{ 0x80000000, 0x80000000, 0x60000000, 0 },
 };
 
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
+	.src_reg	= IMX7D_SRC_SCR,
+	.src_mask	= IMX7D_M4_RST_MASK,
+	.src_start	= IMX7D_M4_START,
+	.src_stop	= IMX7D_M4_STOP,
+	.att		= imx_rproc_att_imx8mq,
+	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mq),
+	.elf_mem_hook	= true,
+};
+
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
 	.src_reg	= IMX7D_SRC_SCR,
 	.src_mask	= IMX7D_M4_RST_MASK,
@@ -513,6 +551,8 @@ static int imx_rproc_remove(struct platform_device *pdev)
 static const struct of_device_id imx_rproc_of_match[] = {
 	{ .compatible = "fsl,imx7d-cm4", .data = &imx_rproc_cfg_imx7d },
 	{ .compatible = "fsl,imx6sx-cm4", .data = &imx_rproc_cfg_imx6sx },
+	{ .compatible = "fsl,imx8mq-cm4", .data = &imx_rproc_cfg_imx8mq },
+	{ .compatible = "fsl,imx8mm-cm4", .data = &imx_rproc_cfg_imx8mq },
 	{},
 };
 MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
-- 
2.28.0

[PATCH V3 7/7] remoteproc: imx_proc: enable virtio/mailbox

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Use virtio/mailbox to build connection between Remote Proccessors
and Linux. Add work queue to handle incoming messages.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 133 ++++++++++++++++++++++++++++++++-
 1 file changed, 130 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index afa650610996..584584a00921 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mailbox_client.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -16,6 +17,9 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
+#include <linux/workqueue.h>
+
+#include "remoteproc_internal.h"
 
 #define IMX7D_SRC_SCR			0x0C
 #define IMX7D_ENABLE_M4			BIT(3)
@@ -88,6 +92,11 @@ struct imx_rproc {
 	const struct imx_rproc_dcfg	*dcfg;
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
+	struct mbox_client		cl;
+	struct mbox_chan		*tx_ch;
+	struct mbox_chan		*rx_ch;
+	struct work_struct		rproc_work;
+	struct workqueue_struct		*workqueue;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
@@ -369,9 +378,33 @@ static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
+static void imx_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct imx_rproc *priv = rproc->priv;
+	int err;
+	__u32 mmsg;
+
+	if (!priv->tx_ch) {
+		dev_err(priv->dev, "No initialized mbox tx channel\n");
+		return;
+	}
+
+	/*
+	 * Send the index of the triggered virtqueue as the mu payload.
+	 * Let remote processor know which virtqueue is used.
+	 */
+	mmsg = vqid << 16;
+
+	err = mbox_send_message(priv->tx_ch, (void *)&mmsg);
+	if (err < 0)
+		dev_err(priv->dev, "%s: failed (%d, err:%d)\n",
+			__func__, vqid, err);
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
+	.kick		= imx_rproc_kick,
 	.da_to_va       = imx_rproc_da_to_va,
 	.load		= rproc_elf_load_segments,
 	.parse_fw	= imx_rproc_parse_fw,
@@ -454,6 +487,77 @@ static void imx_rproc_memset(struct rproc *rproc, void *s, int c, size_t count)
 	memset_io((void * __iomem)s, c, count);
 }
 
+static void imx_rproc_vq_work(struct work_struct *work)
+{
+	struct imx_rproc *priv = container_of(work, struct imx_rproc,
+					      rproc_work);
+
+	rproc_vq_interrupt(priv->rproc, 0);
+	rproc_vq_interrupt(priv->rproc, 1);
+}
+
+static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
+{
+	struct rproc *rproc = dev_get_drvdata(cl->dev);
+	struct imx_rproc *priv = rproc->priv;
+
+	queue_work(priv->workqueue, &priv->rproc_work);
+}
+
+static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	struct device *dev = priv->dev;
+	struct mbox_client *cl;
+	int ret = 0;
+
+	if (!of_get_property(dev->of_node, "mbox-names", NULL))
+		return 0;
+
+	cl = &priv->cl;
+	cl->dev = dev;
+	cl->tx_block = true;
+	cl->tx_tout = 100;
+	cl->knows_txdone = false;
+	cl->rx_callback = imx_rproc_rx_callback;
+
+	priv->tx_ch = mbox_request_channel_byname(cl, "tx");
+	if (IS_ERR(priv->tx_ch)) {
+		if (PTR_ERR(priv->tx_ch) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		ret = PTR_ERR(priv->tx_ch);
+		dev_dbg(cl->dev, "failed to request mbox tx chan, ret %d\n",
+			ret);
+		goto err_out;
+	}
+
+	priv->rx_ch = mbox_request_channel_byname(cl, "rx");
+	if (IS_ERR(priv->rx_ch)) {
+		ret = PTR_ERR(priv->rx_ch);
+		dev_dbg(cl->dev, "failed to request mbox rx chan, ret %d\n",
+			ret);
+		goto err_out;
+	}
+
+	return ret;
+
+err_out:
+	if (!IS_ERR(priv->tx_ch))
+		mbox_free_channel(priv->tx_ch);
+	if (!IS_ERR(priv->rx_ch))
+		mbox_free_channel(priv->rx_ch);
+
+	return ret;
+}
+
+static void imx_rproc_free_mbox(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+
+	mbox_free_channel(priv->tx_ch);
+	mbox_free_channel(priv->rx_ch);
+}
+
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -496,18 +600,31 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	priv->dev = dev;
 
 	dev_set_drvdata(dev, rproc);
+	priv->workqueue = create_workqueue(dev_name(dev));
+	if (!priv->workqueue) {
+		dev_err(dev, "cannot create workqueue\n");
+		ret = -ENOMEM;
+		goto err_put_rproc;
+	}
+
+	ret = imx_rproc_xtr_mbox_init(rproc);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			goto err_put_wkq;
+		/* mbox is optional, so not fail here */
+	}
 
 	ret = imx_rproc_addr_init(priv, pdev);
 	if (ret) {
 		dev_err(dev, "failed on imx_rproc_addr_init\n");
-		goto err_put_rproc;
+		goto err_put_mbox;
 	}
 
 	priv->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to get clock\n");
 		ret = PTR_ERR(priv->clk);
-		goto err_put_rproc;
+		goto err_put_mbox;
 	}
 
 	/*
@@ -517,9 +634,11 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(priv->clk);
 	if (ret) {
 		dev_err(&rproc->dev, "Failed to enable clock\n");
-		goto err_put_rproc;
+		goto err_put_mbox;
 	}
 
+	INIT_WORK(&(priv->rproc_work), imx_rproc_vq_work);
+
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
@@ -530,6 +649,13 @@ static int imx_rproc_probe(struct platform_device *pdev)
 
 err_put_clk:
 	clk_disable_unprepare(priv->clk);
+err_put_mbox:
+	if (!IS_ERR(priv->tx_ch))
+		mbox_free_channel(priv->tx_ch);
+	if (!IS_ERR(priv->rx_ch))
+		mbox_free_channel(priv->rx_ch);
+err_put_wkq:
+	destroy_workqueue(priv->workqueue);
 err_put_rproc:
 	rproc_free(rproc);
 
@@ -542,6 +668,7 @@ static int imx_rproc_remove(struct platform_device *pdev)
 	struct imx_rproc *priv = rproc->priv;
 
 	clk_disable_unprepare(priv->clk);
+	imx_rproc_free_mbox(rproc);
 	rproc_del(rproc);
 	rproc_free(rproc);
 
-- 
2.28.0

Re: [PATCH V3 7/7] remoteproc: imx_proc: enable virtio/mailbox

[Bjorn Andersson]

On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> Use virtio/mailbox to build connection between Remote Proccessors
> and Linux. Add work queue to handle incoming messages.
> 
> Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 133 ++++++++++++++++++++++++++++++++-
>  1 file changed, 130 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index afa650610996..584584a00921 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -8,6 +8,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/mailbox_client.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> @@ -16,6 +17,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
> +#include <linux/workqueue.h>
> +
> +#include "remoteproc_internal.h"
>  
>  #define IMX7D_SRC_SCR			0x0C
>  #define IMX7D_ENABLE_M4			BIT(3)
> @@ -88,6 +92,11 @@ struct imx_rproc {
>  	const struct imx_rproc_dcfg	*dcfg;
>  	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
>  	struct clk			*clk;
> +	struct mbox_client		cl;
> +	struct mbox_chan		*tx_ch;
> +	struct mbox_chan		*rx_ch;
> +	struct work_struct		rproc_work;
> +	struct workqueue_struct		*workqueue;
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
> @@ -369,9 +378,33 @@ static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>  	return 0;
>  }
>  
> +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	int err;
> +	__u32 mmsg;
> +
> +	if (!priv->tx_ch) {
> +		dev_err(priv->dev, "No initialized mbox tx channel\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Send the index of the triggered virtqueue as the mu payload.
> +	 * Let remote processor know which virtqueue is used.
> +	 */
> +	mmsg = vqid << 16;
> +
> +	err = mbox_send_message(priv->tx_ch, (void *)&mmsg);
> +	if (err < 0)
> +		dev_err(priv->dev, "%s: failed (%d, err:%d)\n",
> +			__func__, vqid, err);
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
> +	.kick		= imx_rproc_kick,
>  	.da_to_va       = imx_rproc_da_to_va,
>  	.load		= rproc_elf_load_segments,
>  	.parse_fw	= imx_rproc_parse_fw,
> @@ -454,6 +487,77 @@ static void imx_rproc_memset(struct rproc *rproc, void *s, int c, size_t count)
>  	memset_io((void * __iomem)s, c, count);
>  }
>  
> +static void imx_rproc_vq_work(struct work_struct *work)
> +{
> +	struct imx_rproc *priv = container_of(work, struct imx_rproc,
> +					      rproc_work);
> +
> +	rproc_vq_interrupt(priv->rproc, 0);
> +	rproc_vq_interrupt(priv->rproc, 1);
> +}
> +
> +static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
> +{
> +	struct rproc *rproc = dev_get_drvdata(cl->dev);
> +	struct imx_rproc *priv = rproc->priv;
> +
> +	queue_work(priv->workqueue, &priv->rproc_work);
> +}
> +
> +static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	struct device *dev = priv->dev;
> +	struct mbox_client *cl;
> +	int ret = 0;
> +
> +	if (!of_get_property(dev->of_node, "mbox-names", NULL))
> +		return 0;
> +
> +	cl = &priv->cl;
> +	cl->dev = dev;
> +	cl->tx_block = true;
> +	cl->tx_tout = 100;
> +	cl->knows_txdone = false;
> +	cl->rx_callback = imx_rproc_rx_callback;
> +
> +	priv->tx_ch = mbox_request_channel_byname(cl, "tx");
> +	if (IS_ERR(priv->tx_ch)) {
> +		if (PTR_ERR(priv->tx_ch) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		ret = PTR_ERR(priv->tx_ch);
> +		dev_dbg(cl->dev, "failed to request mbox tx chan, ret %d\n",
> +			ret);

This is worse than a dev_dbg(), something is actually wrong.
Also there's no point in jumping to err_out here, because tx_ch is
IS_ERR() so we're going to skip the first part and rx_ch is not IS_ERR()
so you're going to call mbox_free_channel(NULL) and then return.

So just replace this entire block with:

		return dev_err_probe(dev, PTR_ERR(priv->ch_ch),
				     "failed to request tx mailbox channel: %d\n",
				     ret);
	

> +		goto err_out;
> +	}
> +
> +	priv->rx_ch = mbox_request_channel_byname(cl, "rx");
> +	if (IS_ERR(priv->rx_ch)) {
> +		ret = PTR_ERR(priv->rx_ch);
> +		dev_dbg(cl->dev, "failed to request mbox rx chan, ret %d\n",
> +			ret);
> +		goto err_out;

		mbox_free_channel(priv->tx_ch);
		return dev_err_probe(dev, PTR_ERR(priv->ch_ch),
				     "failed to request rx mailbox channel: %d\n",
				     ret);

> +	}
> +
> +	return ret;

ret is 0 here.

> +
> +err_out:
> +	if (!IS_ERR(priv->tx_ch))
> +		mbox_free_channel(priv->tx_ch);
> +	if (!IS_ERR(priv->rx_ch))
> +		mbox_free_channel(priv->rx_ch);
> +
> +	return ret;
> +}
> +
> +static void imx_rproc_free_mbox(struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +
> +	mbox_free_channel(priv->tx_ch);
> +	mbox_free_channel(priv->rx_ch);
> +}
> +
>  static int imx_rproc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -496,18 +600,31 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	priv->dev = dev;
>  
>  	dev_set_drvdata(dev, rproc);
> +	priv->workqueue = create_workqueue(dev_name(dev));
> +	if (!priv->workqueue) {
> +		dev_err(dev, "cannot create workqueue\n");
> +		ret = -ENOMEM;
> +		goto err_put_rproc;
> +	}
> +
> +	ret = imx_rproc_xtr_mbox_init(rproc);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			goto err_put_wkq;
> +		/* mbox is optional, so not fail here */

imx_rproc_xtr_mbox_init() returns 0 if no mbox was specified, that means
that in all cases that we reach here mailboxes where specified but an
error occurred. You should not ignore this.

> +	}
>  
>  	ret = imx_rproc_addr_init(priv, pdev);
>  	if (ret) {
>  		dev_err(dev, "failed on imx_rproc_addr_init\n");
> -		goto err_put_rproc;
> +		goto err_put_mbox;
>  	}
>  
>  	priv->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		dev_err(dev, "Failed to get clock\n");
>  		ret = PTR_ERR(priv->clk);
> -		goto err_put_rproc;
> +		goto err_put_mbox;
>  	}
>  
>  	/*
> @@ -517,9 +634,11 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	ret = clk_prepare_enable(priv->clk);
>  	if (ret) {
>  		dev_err(&rproc->dev, "Failed to enable clock\n");
> -		goto err_put_rproc;
> +		goto err_put_mbox;
>  	}
>  
> +	INIT_WORK(&(priv->rproc_work), imx_rproc_vq_work);
> +
>  	ret = rproc_add(rproc);
>  	if (ret) {
>  		dev_err(dev, "rproc_add failed\n");
> @@ -530,6 +649,13 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  
>  err_put_clk:
>  	clk_disable_unprepare(priv->clk);
> +err_put_mbox:
> +	if (!IS_ERR(priv->tx_ch))

With above changes you won't get here with IS_ERR(tx_ch) ||
IS_ERR(rx_ch), so you can safely remove the conditionals and just call
mbox_free_channel().

Regards,
Bjorn

> +		mbox_free_channel(priv->tx_ch);
> +	if (!IS_ERR(priv->rx_ch))
> +		mbox_free_channel(priv->rx_ch);
> +err_put_wkq:
> +	destroy_workqueue(priv->workqueue);
>  err_put_rproc:
>  	rproc_free(rproc);
>  
> @@ -542,6 +668,7 @@ static int imx_rproc_remove(struct platform_device *pdev)
>  	struct imx_rproc *priv = rproc->priv;
>  
>  	clk_disable_unprepare(priv->clk);
> +	imx_rproc_free_mbox(rproc);
>  	rproc_del(rproc);
>  	rproc_free(rproc);
>  
> -- 
> 2.28.0
> 

RE: [PATCH V3 7/7] remoteproc: imx_proc: enable virtio/mailbox

[Peng Fan]

> Subject: Re: [PATCH V3 7/7] remoteproc: imx_proc: enable virtio/mailbox
> 
> On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> 
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Use virtio/mailbox to build connection between Remote Proccessors and
> > Linux. Add work queue to handle incoming messages.
> >
> > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 133
> > ++++++++++++++++++++++++++++++++-
> >  1 file changed, 130 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index afa650610996..584584a00921
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mailbox_client.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/module.h>
> >  #include <linux/of_address.h>
> > @@ -16,6 +17,9 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> >  #include <linux/remoteproc.h>
> > +#include <linux/workqueue.h>
> > +
> > +#include "remoteproc_internal.h"
> >
> >  #define IMX7D_SRC_SCR			0x0C
> >  #define IMX7D_ENABLE_M4			BIT(3)
> > @@ -88,6 +92,11 @@ struct imx_rproc {
> >  	const struct imx_rproc_dcfg	*dcfg;
> >  	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
> >  	struct clk			*clk;
> > +	struct mbox_client		cl;
> > +	struct mbox_chan		*tx_ch;
> > +	struct mbox_chan		*rx_ch;
> > +	struct work_struct		rproc_work;
> > +	struct workqueue_struct		*workqueue;
> >  };
> >
> >  static const struct imx_rproc_att imx_rproc_att_imx8mq[] = { @@
> > -369,9 +378,33 @@ static int imx_rproc_parse_fw(struct rproc *rproc,
> const struct firmware *fw)
> >  	return 0;
> >  }
> >
> > +static void imx_rproc_kick(struct rproc *rproc, int vqid) {
> > +	struct imx_rproc *priv = rproc->priv;
> > +	int err;
> > +	__u32 mmsg;
> > +
> > +	if (!priv->tx_ch) {
> > +		dev_err(priv->dev, "No initialized mbox tx channel\n");
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Send the index of the triggered virtqueue as the mu payload.
> > +	 * Let remote processor know which virtqueue is used.
> > +	 */
> > +	mmsg = vqid << 16;
> > +
> > +	err = mbox_send_message(priv->tx_ch, (void *)&mmsg);
> > +	if (err < 0)
> > +		dev_err(priv->dev, "%s: failed (%d, err:%d)\n",
> > +			__func__, vqid, err);
> > +}
> > +
> >  static const struct rproc_ops imx_rproc_ops = {
> >  	.start		= imx_rproc_start,
> >  	.stop		= imx_rproc_stop,
> > +	.kick		= imx_rproc_kick,
> >  	.da_to_va       = imx_rproc_da_to_va,
> >  	.load		= rproc_elf_load_segments,
> >  	.parse_fw	= imx_rproc_parse_fw,
> > @@ -454,6 +487,77 @@ static void imx_rproc_memset(struct rproc *rproc,
> void *s, int c, size_t count)
> >  	memset_io((void * __iomem)s, c, count);  }
> >
> > +static void imx_rproc_vq_work(struct work_struct *work) {
> > +	struct imx_rproc *priv = container_of(work, struct imx_rproc,
> > +					      rproc_work);
> > +
> > +	rproc_vq_interrupt(priv->rproc, 0);
> > +	rproc_vq_interrupt(priv->rproc, 1);
> > +}
> > +
> > +static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
> > +{
> > +	struct rproc *rproc = dev_get_drvdata(cl->dev);
> > +	struct imx_rproc *priv = rproc->priv;
> > +
> > +	queue_work(priv->workqueue, &priv->rproc_work); }
> > +
> > +static int imx_rproc_xtr_mbox_init(struct rproc *rproc) {
> > +	struct imx_rproc *priv = rproc->priv;
> > +	struct device *dev = priv->dev;
> > +	struct mbox_client *cl;
> > +	int ret = 0;
> > +
> > +	if (!of_get_property(dev->of_node, "mbox-names", NULL))
> > +		return 0;
> > +
> > +	cl = &priv->cl;
> > +	cl->dev = dev;
> > +	cl->tx_block = true;
> > +	cl->tx_tout = 100;
> > +	cl->knows_txdone = false;
> > +	cl->rx_callback = imx_rproc_rx_callback;
> > +
> > +	priv->tx_ch = mbox_request_channel_byname(cl, "tx");
> > +	if (IS_ERR(priv->tx_ch)) {
> > +		if (PTR_ERR(priv->tx_ch) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +		ret = PTR_ERR(priv->tx_ch);
> > +		dev_dbg(cl->dev, "failed to request mbox tx chan, ret %d\n",
> > +			ret);
> 
> This is worse than a dev_dbg(), something is actually wrong.
> Also there's no point in jumping to err_out here, because tx_ch is
> IS_ERR() so we're going to skip the first part and rx_ch is not IS_ERR() so
> you're going to call mbox_free_channel(NULL) and then return.
> 
> So just replace this entire block with:
> 
> 		return dev_err_probe(dev, PTR_ERR(priv->ch_ch),
> 				     "failed to request tx mailbox channel: %d\n",
> 				     ret);

Yeah. Thanks.

> 
> 
> > +		goto err_out;
> > +	}
> > +
> > +	priv->rx_ch = mbox_request_channel_byname(cl, "rx");
> > +	if (IS_ERR(priv->rx_ch)) {
> > +		ret = PTR_ERR(priv->rx_ch);
> > +		dev_dbg(cl->dev, "failed to request mbox rx chan, ret %d\n",
> > +			ret);
> > +		goto err_out;
> 
> 		mbox_free_channel(priv->tx_ch);
> 		return dev_err_probe(dev, PTR_ERR(priv->ch_ch),
> 				     "failed to request rx mailbox channel: %d\n",
> 				     ret);
> 
> > +	}
> > +
> > +	return ret;
> 
> ret is 0 here.

Yes.

> 
> > +
> > +err_out:
> > +	if (!IS_ERR(priv->tx_ch))
> > +		mbox_free_channel(priv->tx_ch);
> > +	if (!IS_ERR(priv->rx_ch))
> > +		mbox_free_channel(priv->rx_ch);
> > +
> > +	return ret;
> > +}
> > +
> > +static void imx_rproc_free_mbox(struct rproc *rproc) {
> > +	struct imx_rproc *priv = rproc->priv;
> > +
> > +	mbox_free_channel(priv->tx_ch);
> > +	mbox_free_channel(priv->rx_ch);
> > +}
> > +
> >  static int imx_rproc_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> > @@ -496,18 +600,31 @@ static int imx_rproc_probe(struct
> platform_device *pdev)
> >  	priv->dev = dev;
> >
> >  	dev_set_drvdata(dev, rproc);
> > +	priv->workqueue = create_workqueue(dev_name(dev));
> > +	if (!priv->workqueue) {
> > +		dev_err(dev, "cannot create workqueue\n");
> > +		ret = -ENOMEM;
> > +		goto err_put_rproc;
> > +	}
> > +
> > +	ret = imx_rproc_xtr_mbox_init(rproc);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER)
> > +			goto err_put_wkq;
> > +		/* mbox is optional, so not fail here */
> 
> imx_rproc_xtr_mbox_init() returns 0 if no mbox was specified, that means
> that in all cases that we reach here mailboxes where specified but an error
> occurred. You should not ignore this.

ok, fix in v4.

> 
> > +	}
> >
> >  	ret = imx_rproc_addr_init(priv, pdev);
> >  	if (ret) {
> >  		dev_err(dev, "failed on imx_rproc_addr_init\n");
> > -		goto err_put_rproc;
> > +		goto err_put_mbox;
> >  	}
> >
> >  	priv->clk = devm_clk_get(dev, NULL);
> >  	if (IS_ERR(priv->clk)) {
> >  		dev_err(dev, "Failed to get clock\n");
> >  		ret = PTR_ERR(priv->clk);
> > -		goto err_put_rproc;
> > +		goto err_put_mbox;
> >  	}
> >
> >  	/*
> > @@ -517,9 +634,11 @@ static int imx_rproc_probe(struct platform_device
> *pdev)
> >  	ret = clk_prepare_enable(priv->clk);
> >  	if (ret) {
> >  		dev_err(&rproc->dev, "Failed to enable clock\n");
> > -		goto err_put_rproc;
> > +		goto err_put_mbox;
> >  	}
> >
> > +	INIT_WORK(&(priv->rproc_work), imx_rproc_vq_work);
> > +
> >  	ret = rproc_add(rproc);
> >  	if (ret) {
> >  		dev_err(dev, "rproc_add failed\n"); @@ -530,6 +649,13 @@ static
> int
> > imx_rproc_probe(struct platform_device *pdev)
> >
> >  err_put_clk:
> >  	clk_disable_unprepare(priv->clk);
> > +err_put_mbox:
> > +	if (!IS_ERR(priv->tx_ch))
> 
> With above changes you won't get here with IS_ERR(tx_ch) || IS_ERR(rx_ch),
> so you can safely remove the conditionals and just call mbox_free_channel().


Yes.

After we agree on the new method of patch 1/7, I'll post out v4.

Thanks,
Peng.

> 
> Regards,
> Bjorn
> 
> > +		mbox_free_channel(priv->tx_ch);
> > +	if (!IS_ERR(priv->rx_ch))
> > +		mbox_free_channel(priv->rx_ch);
> > +err_put_wkq:
> > +	destroy_workqueue(priv->workqueue);
> >  err_put_rproc:
> >  	rproc_free(rproc);
> >
> > @@ -542,6 +668,7 @@ static int imx_rproc_remove(struct platform_device
> *pdev)
> >  	struct imx_rproc *priv = rproc->priv;
> >
> >  	clk_disable_unprepare(priv->clk);
> > +	imx_rproc_free_mbox(rproc);
> >  	rproc_del(rproc);
> >  	rproc_free(rproc);
> >
> > --
> > 2.28.0
> >