linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] remoteproc: imx_rproc: dummy kick method
@ 2020-03-04 14:26 Nikita Shubin
  2020-03-04 14:26 ` [PATCH 2/2] remoteproc: imx_rproc: set pc on start Nikita Shubin
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Nikita Shubin @ 2020-03-04 14:26 UTC (permalink / raw)
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

add kick method that does nothing, to avoid errors in rproc_virtio_notify.

Signed-off-by: Nikita Shubin <NShubin@topcon.com>
---
 drivers/remoteproc/imx_rproc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 3e72b6f38d4b..796b6b86550a 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -240,9 +240,15 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 	return va;
 }
 
+static void imx_rproc_kick(struct rproc *rproc, int vqid)
+{
+
+}
+
 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,
 };
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 2/2] remoteproc: imx_rproc: set pc on start
  2020-03-04 14:26 [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Nikita Shubin
@ 2020-03-04 14:26 ` Nikita Shubin
  2020-03-05 16:16 ` [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Mathieu Poirier
  2020-04-06 11:33 ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support nikita.shubin
  2 siblings, 0 replies; 29+ messages in thread
From: Nikita Shubin @ 2020-03-04 14:26 UTC (permalink / raw)
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

In case elf file interrupt vector is not supposed to be at OCRAM_S,
it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
firmware.

Otherwise firmware located anywhere besides OCRAM_S won't boot.

The firmware must set stack poiner as first instruction:

Reset_Handler:
    ldr   sp, = __stack      /* set stack pointer */

Signed-off-by: Nikita Shubin <NShubin@topcon.com>
---
 drivers/remoteproc/imx_rproc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 796b6b86550a..d02007f05ebd 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -45,6 +45,8 @@
 
 #define IMX7D_RPROC_MEM_MAX		8
 
+#define IMX_BOOT_PC			0x4
+
 /**
  * struct imx_rproc_mem - slim internal memory structure
  * @cpu_addr: MPU virtual address of the memory region
@@ -85,6 +87,7 @@ struct imx_rproc {
 	const struct imx_rproc_dcfg	*dcfg;
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
+	void __iomem			*bootreg;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
@@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc *rproc)
 	struct device *dev = priv->dev;
 	int ret;
 
+	/* write entry point to program counter */
+	writel(rproc->bootaddr, priv->bootreg);
+
 	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
 				 dcfg->src_mask, dcfg->src_start);
 	if (ret)
 		dev_err(dev, "Failed to enable M4!\n");
 
+	dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
+
 	return ret;
 }
 
@@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
 	if (ret)
 		dev_err(dev, "Failed to stop M4!\n");
 
+	/* clear entry points */
+	writel(0, priv->bootreg);
+
 	return ret;
 }
 
@@ -366,6 +377,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_rproc;
 	}
 
+	priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC, sizeof(u32));
+
 	/*
 	 * clk for M4 block including memory. Should be
 	 * enabled before .start for FW transfer.
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] remoteproc: imx_rproc: dummy kick method
  2020-03-04 14:26 [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Nikita Shubin
  2020-03-04 14:26 ` [PATCH 2/2] remoteproc: imx_rproc: set pc on start Nikita Shubin
@ 2020-03-05 16:16 ` Mathieu Poirier
  2020-03-05 17:29   ` nikita.shubin
  2020-04-06 11:33 ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support nikita.shubin
  2 siblings, 1 reply; 29+ messages in thread
From: Mathieu Poirier @ 2020-03-05 16:16 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-remoteproc, linux-arm-kernel, Linux Kernel Mailing List

On Wed, 4 Mar 2020 at 07:25, Nikita Shubin <NShubin@topcon.com> wrote:
>
> add kick method that does nothing, to avoid errors in rproc_virtio_notify.
>
> Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 3e72b6f38d4b..796b6b86550a 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -240,9 +240,15 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>         return va;
>  }
>
> +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +
> +}
> +

If rproc::kick() is empty, how does the MCU know there is packets to
fetch in the virtio queues?

>  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,
>  };
>
> --
> 2.24.1
>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] remoteproc: imx_rproc: dummy kick method
  2020-03-05 16:16 ` [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Mathieu Poirier
@ 2020-03-05 17:29   ` nikita.shubin
  2020-03-05 17:54     ` Mathieu Poirier
  0 siblings, 1 reply; 29+ messages in thread
From: nikita.shubin @ 2020-03-05 17:29 UTC (permalink / raw)
  To: Mathieu Poirier, Nikita Shubin
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-remoteproc, linux-arm-kernel, Linux Kernel Mailing List



05.03.2020, 19:17, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
> On Wed, 4 Mar 2020 at 07:25, Nikita Shubin <NShubin@topcon.com> wrote:
>>  add kick method that does nothing, to avoid errors in rproc_virtio_notify.
>>
>>  Signed-off-by: Nikita Shubin <NShubin@topcon.com>
>>  ---
>>   drivers/remoteproc/imx_rproc.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>>  diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>>  index 3e72b6f38d4b..796b6b86550a 100644
>>  --- a/drivers/remoteproc/imx_rproc.c
>>  +++ b/drivers/remoteproc/imx_rproc.c
>>  @@ -240,9 +240,15 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>>          return va;
>>   }
>>
>>  +static void imx_rproc_kick(struct rproc *rproc, int vqid)
>>  +{
>>  +
>>  +}
>>  +
>
> If rproc::kick() is empty, how does the MCU know there is packets to
> fetch in the virtio queues?

Well, of course it doesn't i understand this perfectly - just following documentation citing:

| Every remoteproc implementation should at least provide the ->start and ->stop
| handlers. If rpmsg/virtio functionality is also desired, then the ->kick handler
| should be provided as well.

But i as i mentioned in "remoteproc: Fix NULL pointer dereference in rproc_virtio_notify" kick method will be called if 
"resource_table exists in firmware and has "Virtio device entry" defined" anyway, the imx_rproc is not in control of what 
exactly it is booting, so such situation can occur.

>
>>   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,
>>   };
>>
>>  --
>>  2.24.1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] remoteproc: imx_rproc: dummy kick method
  2020-03-05 17:29   ` nikita.shubin
@ 2020-03-05 17:54     ` Mathieu Poirier
  2020-03-05 18:07       ` nikita.shubin
  0 siblings, 1 reply; 29+ messages in thread
From: Mathieu Poirier @ 2020-03-05 17:54 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel,
	Linux Kernel Mailing List

On Thu, 5 Mar 2020 at 10:29, <nikita.shubin@maquefel.me> wrote:
>
>
>
> 05.03.2020, 19:17, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
> > On Wed, 4 Mar 2020 at 07:25, Nikita Shubin <NShubin@topcon.com> wrote:
> >>  add kick method that does nothing, to avoid errors in rproc_virtio_notify.
> >>
> >>  Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> >>  ---
> >>   drivers/remoteproc/imx_rproc.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >>  diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> >>  index 3e72b6f38d4b..796b6b86550a 100644
> >>  --- a/drivers/remoteproc/imx_rproc.c
> >>  +++ b/drivers/remoteproc/imx_rproc.c
> >>  @@ -240,9 +240,15 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
> >>          return va;
> >>   }
> >>
> >>  +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> >>  +{
> >>  +
> >>  +}
> >>  +
> >
> > If rproc::kick() is empty, how does the MCU know there is packets to
> > fetch in the virtio queues?
>
> Well, of course it doesn't i understand this perfectly - just following documentation citing:
>
> | Every remoteproc implementation should at least provide the ->start and ->stop
> | handlers. If rpmsg/virtio functionality is also desired, then the ->kick handler
> | should be provided as well.
>
> But i as i mentioned in "remoteproc: Fix NULL pointer dereference in rproc_virtio_notify" kick method will be called if
> "resource_table exists in firmware and has "Virtio device entry" defined" anyway, the imx_rproc is not in control of what
> exactly it is booting, so such situation can occur.

If I understand correctly, the MCU can boot images that have a virtio
device in its resource table and still do useful work even if the
virtio device/rpmsg bus can't be setup - is this correct?

Thanks,
Mathieu

>
> >
> >>   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,
> >>   };
> >>
> >>  --
> >>  2.24.1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] remoteproc: imx_rproc: dummy kick method
  2020-03-05 17:54     ` Mathieu Poirier
@ 2020-03-05 18:07       ` nikita.shubin
  2020-03-05 18:36         ` Mathieu Poirier
  0 siblings, 1 reply; 29+ messages in thread
From: nikita.shubin @ 2020-03-05 18:07 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel,
	Linux Kernel Mailing List



05.03.2020, 20:54, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
> On Thu, 5 Mar 2020 at 10:29, <nikita.shubin@maquefel.me> wrote:
>>  05.03.2020, 19:17, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
>>  > On Wed, 4 Mar 2020 at 07:25, Nikita Shubin <NShubin@topcon.com> wrote:
>>  >> add kick method that does nothing, to avoid errors in rproc_virtio_notify.
>>  >>
>>  >> Signed-off-by: Nikita Shubin <NShubin@topcon.com>
>>  >> ---
>>  >> drivers/remoteproc/imx_rproc.c | 6 ++++++
>>  >> 1 file changed, 6 insertions(+)
>>  >>
>>  >> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>>  >> index 3e72b6f38d4b..796b6b86550a 100644
>>  >> --- a/drivers/remoteproc/imx_rproc.c
>>  >> +++ b/drivers/remoteproc/imx_rproc.c
>>  >> @@ -240,9 +240,15 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>>  >> return va;
>>  >> }
>>  >>
>>  >> +static void imx_rproc_kick(struct rproc *rproc, int vqid)
>>  >> +{
>>  >> +
>>  >> +}
>>  >> +
>>  >
>>  > If rproc::kick() is empty, how does the MCU know there is packets to
>>  > fetch in the virtio queues?
>>
>>  Well, of course it doesn't i understand this perfectly - just following documentation citing:
>>
>>  | Every remoteproc implementation should at least provide the ->start and ->stop
>>  | handlers. If rpmsg/virtio functionality is also desired, then the ->kick handler
>>  | should be provided as well.
>>
>>  But i as i mentioned in "remoteproc: Fix NULL pointer dereference in rproc_virtio_notify" kick method will be called if
>>  "resource_table exists in firmware and has "Virtio device entry" defined" anyway, the imx_rproc is not in control of what
>>  exactly it is booting, so such situation can occur.
>
> If I understand correctly, the MCU can boot images that have a virtio
> device in its resource table and still do useful work even if the
> virtio device/rpmsg bus can't be setup - is this correct?

Yes, this assumption is correct. 

Despite this situation is not i desire at all - such thing can happen.
I am currently using co-proc as a realtime part of UGV control, 
so it must immediately stop the engines, if not provided with navigational information.

The imx7d MCU have access to the most periphery that have the main processor.

Of course the kick method should do real work, but i decided to submit step by step if i am allowed to do so.

>
> Thanks,
> Mathieu
>
>>  >
>>  >> 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,
>>  >> };
>>  >>
>>  >> --
>>  >> 2.24.1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] remoteproc: imx_rproc: dummy kick method
  2020-03-05 18:07       ` nikita.shubin
@ 2020-03-05 18:36         ` Mathieu Poirier
  2020-03-05 18:46           ` nikita.shubin
  0 siblings, 1 reply; 29+ messages in thread
From: Mathieu Poirier @ 2020-03-05 18:36 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel,
	Linux Kernel Mailing List

On Thu, 5 Mar 2020 at 11:07, <nikita.shubin@maquefel.me> wrote:
>
>
>
> 05.03.2020, 20:54, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
> > On Thu, 5 Mar 2020 at 10:29, <nikita.shubin@maquefel.me> wrote:
> >>  05.03.2020, 19:17, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
> >>  > On Wed, 4 Mar 2020 at 07:25, Nikita Shubin <NShubin@topcon.com> wrote:
> >>  >> add kick method that does nothing, to avoid errors in rproc_virtio_notify.
> >>  >>
> >>  >> Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> >>  >> ---
> >>  >> drivers/remoteproc/imx_rproc.c | 6 ++++++
> >>  >> 1 file changed, 6 insertions(+)
> >>  >>
> >>  >> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> >>  >> index 3e72b6f38d4b..796b6b86550a 100644
> >>  >> --- a/drivers/remoteproc/imx_rproc.c
> >>  >> +++ b/drivers/remoteproc/imx_rproc.c
> >>  >> @@ -240,9 +240,15 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
> >>  >> return va;
> >>  >> }
> >>  >>
> >>  >> +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> >>  >> +{
> >>  >> +
> >>  >> +}
> >>  >> +
> >>  >
> >>  > If rproc::kick() is empty, how does the MCU know there is packets to
> >>  > fetch in the virtio queues?
> >>
> >>  Well, of course it doesn't i understand this perfectly - just following documentation citing:
> >>
> >>  | Every remoteproc implementation should at least provide the ->start and ->stop
> >>  | handlers. If rpmsg/virtio functionality is also desired, then the ->kick handler
> >>  | should be provided as well.
> >>
> >>  But i as i mentioned in "remoteproc: Fix NULL pointer dereference in rproc_virtio_notify" kick method will be called if
> >>  "resource_table exists in firmware and has "Virtio device entry" defined" anyway, the imx_rproc is not in control of what
> >>  exactly it is booting, so such situation can occur.
> >
> > If I understand correctly, the MCU can boot images that have a virtio
> > device in its resource table and still do useful work even if the
> > virtio device/rpmsg bus can't be setup - is this correct?
>
> Yes, this assumption is correct.
>
> Despite this situation is not i desire at all - such thing can happen.
> I am currently using co-proc as a realtime part of UGV control,
> so it must immediately stop the engines, if not provided with navigational information.
>
> The imx7d MCU have access to the most periphery that have the main processor.
>
> Of course the kick method should do real work, but i decided to submit step by step if i am allowed to do so.

Ok, the situation is clearer now and I have put your patches back in
my queue.  I am seriously back-logged at this time so it will take a
little while before I get to them.

>
> >
> > Thanks,
> > Mathieu
> >
> >>  >
> >>  >> 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,
> >>  >> };
> >>  >>
> >>  >> --
> >>  >> 2.24.1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] remoteproc: imx_rproc: dummy kick method
  2020-03-05 18:36         ` Mathieu Poirier
@ 2020-03-05 18:46           ` nikita.shubin
  0 siblings, 0 replies; 29+ messages in thread
From: nikita.shubin @ 2020-03-05 18:46 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel,
	Linux Kernel Mailing List

That's totally okay - thank you for review.

05.03.2020, 21:36, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
> On Thu, 5 Mar 2020 at 11:07, <nikita.shubin@maquefel.me> wrote:
>>  05.03.2020, 20:54, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
>>  > On Thu, 5 Mar 2020 at 10:29, <nikita.shubin@maquefel.me> wrote:
>>  >> 05.03.2020, 19:17, "Mathieu Poirier" <mathieu.poirier@linaro.org>:
>>  >> > On Wed, 4 Mar 2020 at 07:25, Nikita Shubin <NShubin@topcon.com> wrote:
>>  >> >> add kick method that does nothing, to avoid errors in rproc_virtio_notify.
>>  >> >>
>>  >> >> Signed-off-by: Nikita Shubin <NShubin@topcon.com>
>>  >> >> ---
>>  >> >> drivers/remoteproc/imx_rproc.c | 6 ++++++
>>  >> >> 1 file changed, 6 insertions(+)
>>  >> >>
>>  >> >> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>>  >> >> index 3e72b6f38d4b..796b6b86550a 100644
>>  >> >> --- a/drivers/remoteproc/imx_rproc.c
>>  >> >> +++ b/drivers/remoteproc/imx_rproc.c
>>  >> >> @@ -240,9 +240,15 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>>  >> >> return va;
>>  >> >> }
>>  >> >>
>>  >> >> +static void imx_rproc_kick(struct rproc *rproc, int vqid)
>>  >> >> +{
>>  >> >> +
>>  >> >> +}
>>  >> >> +
>>  >> >
>>  >> > If rproc::kick() is empty, how does the MCU know there is packets to
>>  >> > fetch in the virtio queues?
>>  >>
>>  >> Well, of course it doesn't i understand this perfectly - just following documentation citing:
>>  >>
>>  >> | Every remoteproc implementation should at least provide the ->start and ->stop
>>  >> | handlers. If rpmsg/virtio functionality is also desired, then the ->kick handler
>>  >> | should be provided as well.
>>  >>
>>  >> But i as i mentioned in "remoteproc: Fix NULL pointer dereference in rproc_virtio_notify" kick method will be called if
>>  >> "resource_table exists in firmware and has "Virtio device entry" defined" anyway, the imx_rproc is not in control of what
>>  >> exactly it is booting, so such situation can occur.
>>  >
>>  > If I understand correctly, the MCU can boot images that have a virtio
>>  > device in its resource table and still do useful work even if the
>>  > virtio device/rpmsg bus can't be setup - is this correct?
>>
>>  Yes, this assumption is correct.
>>
>>  Despite this situation is not i desire at all - such thing can happen.
>>  I am currently using co-proc as a realtime part of UGV control,
>>  so it must immediately stop the engines, if not provided with navigational information.
>>
>>  The imx7d MCU have access to the most periphery that have the main processor.
>>
>>  Of course the kick method should do real work, but i decided to submit step by step if i am allowed to do so.
>
> Ok, the situation is clearer now and I have put your patches back in
> my queue. I am seriously back-logged at this time so it will take a
> little while before I get to them.
>
>>  >
>>  > Thanks,
>>  > Mathieu
>>  >
>>  >> >
>>  >> >> 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,
>>  >> >> };
>>  >> >>
>>  >> >> --
>>  >> >> 2.24.1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support
  2020-03-04 14:26 [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Nikita Shubin
  2020-03-04 14:26 ` [PATCH 2/2] remoteproc: imx_rproc: set pc on start Nikita Shubin
  2020-03-05 16:16 ` [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Mathieu Poirier
@ 2020-04-06 11:33 ` nikita.shubin
  2020-04-06 11:33   ` [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start nikita.shubin
                     ` (3 more replies)
  2 siblings, 4 replies; 29+ messages in thread
From: nikita.shubin @ 2020-04-06 11:33 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-remoteproc, linux-arm-kernel, linux-kernel

This patch set introduces virtio support for imx7d-m4 communication:

- support booting loaded vim imx-rproc firmware
- implement .kick method support using mailbox in imx-processor
- parse vdev0vring0, vdev0vring1, vdev0buffer memory regions required
for virtio_rpmsg_bus initialization

Regarding imx7d-m4 boot proccess

Citing ARM documentation:

At Reset, Cortex-M3 and Cortex-M4 processors always boot from a vector table at address zero.

"With uninitialized memory at address zero (for example, unprogrammed Flash or uninitialized RAM),
the processor will read a spurious initial Main Stack Pointer value from address zero and a spurious
code entry point (Reset vector) from address 0x4, possibly containing an illegal instruction
set state specifier (ESPR.T bit) in bit[0]."

So to successfully boot m4 coproc we need to write Stack Pointer and Program counter, i see no
obvious to get Stack Pointer value, so two ways exist ethier form a special elf section:

"
.loader :
  {
    LONG(__StackTop);
    LONG(Reset_Handler + 1);
  } > m_start
"

and put it at 0x0 address:

"
m_start               (RX)  : ORIGIN = 0x00000000, LENGTH = 0x00008000
"

Or (the way i've chosen) only put Entry Point at 0x04 and set stack as first instruction:

"
Reset_Handler:
	ldr   sp, =__stack      /* set stack pointer */
"

Regarding mailboxes and memory regions :

This code is heavily derived from stm32-rproc (i.e. copy pasted) and this fact needs to
reflected in commits, please tell me how to emphasize this fact.

Attaching succesful trace booting m4 (with Add rpmsg tty driver applied) :

[  143.240616] remoteproc remoteproc0: powering up imx-rproc
[  143.251768] remoteproc remoteproc0: Booting fw image huginn.elf, size 466876
[  143.251786] imx-rproc imx7d-cm4: iommu not present
[  143.251825] remoteproc remoteproc0: rsc: type 3
[  143.251837] remoteproc remoteproc0: vdev rsc: id 7, dfeatures 0x1, cfg len 0, 2 vrings
[  143.251924] remoteproc remoteproc0: vdev rsc: vring0: da 0xffffffff, qsz 16, align 16
[  143.251935] remoteproc remoteproc0: vdev rsc: vring1: da 0xffffffff, qsz 16, align 16
[  143.251955] imx-rproc imx7d-cm4: map memory: 0x00900000+20000
[  143.251987] imx-rproc imx7d-cm4: map memory: 0x00920000+2000
[  143.252003] imx-rproc imx7d-cm4: map memory: 0x00922000+2000
[  143.252020] remoteproc remoteproc0: phdr: type 1 da 0x20200000 memsz 0x240 filesz 0x240
[  143.252032] remoteproc remoteproc0: da = 0x20200000 len = 0x240 va = 0x(ptrval)
[  143.252043] remoteproc remoteproc0: phdr: type 1 da 0x20200240 memsz 0x5b38 filesz 0x5b38
[  143.252053] remoteproc remoteproc0: da = 0x20200240 len = 0x5b38 va = 0x(ptrval)
[  143.252105] remoteproc remoteproc0: phdr: type 1 da 0x20205d78 memsz 0x4b58 filesz 0x758
[  143.252115] remoteproc remoteproc0: da = 0x20205d78 len = 0x4b58 va = 0x(ptrval)
[  143.252159] remoteproc remoteproc0: da = 0x200006cc len = 0x8c va = 0x(ptrval)
[  143.252176] remoteproc remoteproc0: Started from 0x202002f5
[  143.252211]  imx7d-cm4#vdev0buffer: assigned reserved memory node vdev0buffer@00924000
[  143.252232] virtio virtio0: reset !
[  143.252241] virtio virtio0: status: 1
[  143.260567] virtio_rpmsg_bus virtio0: status: 3
[  143.260598] remoteproc remoteproc0: vring0: va c083c000 qsz 16 notifyid 0
[  143.260614] remoteproc remoteproc0: vring1: va c0872000 qsz 16 notifyid 1
[  143.260651] virtio_rpmsg_bus virtio0: buffers: va c0894000, dma 0x00924000
[  143.260666] Added buffer head 0 to (ptrval)
[  143.260674] Added buffer head 1 to (ptrval)
[  143.260680] Added buffer head 2 to (ptrval)
[  143.260686] Added buffer head 3 to (ptrval)
[  143.260692] Added buffer head 4 to (ptrval)
[  143.260697] Added buffer head 5 to (ptrval)
[  143.260703] Added buffer head 6 to (ptrval)
[  143.260709] Added buffer head 7 to (ptrval)
[  143.260715] Added buffer head 8 to (ptrval)
[  143.260721] Added buffer head 9 to (ptrval)
[  143.260727] Added buffer head 10 to (ptrval)
[  143.260733] Added buffer head 11 to (ptrval)
[  143.260738] Added buffer head 12 to (ptrval)
[  143.260744] Added buffer head 13 to (ptrval)
[  143.260750] Added buffer head 14 to (ptrval)
[  143.260756] Added buffer head 15 to (ptrval)
[  143.260771] virtio_rpmsg_bus virtio0: status: 7
[  143.260779] remoteproc remoteproc0: kicking vq index: 0
[  143.260788] remoteproc remoteproc0: sending message : vqid = 0
[  143.260802] imx_mu 30aa0000.mailbox: Send data on wrong channel type: 1
[  143.260810] virtio_rpmsg_bus virtio0: rpmsg host is online
[  143.261680]  imx7d-cm4#vdev0buffer: registered virtio0 (type 7)
[  143.261694] remoteproc remoteproc0: remote processor imx-rproc is now up
[  143.354880] remoteproc remoteproc0: vq index 0 is interrupted
[  143.354895] virtqueue callback for (ptrval) ((ptrval))
[  143.354912] virtio_rpmsg_bus virtio0: From: 0x0, To: 0x35, Len: 40, Flags: 0, Reserved: 0
[  143.354924] rpmsg_virtio RX: 00 00 00 00 35 00 00 00 00 00 00 00 28 00 00 00  ....5.......(...
[  143.354932] rpmsg_virtio RX: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00 00 00  rpmsg-tty-raw...
[  143.354939] rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  143.354945] rpmsg_virtio RX: 00 00 00 00 00 00 00 00                          ........
[  143.354956] NS announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00 00 00  rpmsg-tty-raw...
[  143.354963] NS announcement: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  143.354969] NS announcement: 00 00 00 00 00 00 00 00                          ........
[  143.354980] virtio_rpmsg_bus virtio0: creating channel rpmsg-tty-raw addr 0x0
[  143.356584] rpmsg_tty virtio0.rpmsg-tty-raw.-1.0: new channel: 0x400 -> 0x0 : ttyRPMSG0
[  143.356651] Added buffer head 0 to (ptrval)
[  143.356658] No more buffers in queue
[  143.356667] virtio_rpmsg_bus virtio0: Received 1 messages
[  143.404302] remoteproc remoteproc0: vq index 0 is interrupted
[  143.404319] virtqueue callback for (ptrval) ((ptrval))
[  143.404337] virtio_rpmsg_bus virtio0: From: 0x1, To: 0x35, Len: 40, Flags: 0, Reserved: 0
[  143.404350] rpmsg_virtio RX: 01 00 00 00 35 00 00 00 00 00 00 00 28 00 00 00  ....5.......(...
[  143.404391] rpmsg_virtio RX: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00 00 00  rpmsg-tty-raw...
[  143.404399] rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  143.404405] rpmsg_virtio RX: 01 00 00 00 00 00 00 00                          ........
[  143.404417] NS announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00 00 00  rpmsg-tty-raw...
[  143.404424] NS announcement: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  143.404430] NS announcement: 01 00 00 00 00 00 00 00                          ........
[  143.404441] virtio_rpmsg_bus virtio0: creating channel rpmsg-tty-raw addr 0x1
[  143.411114] rpmsg_tty virtio0.rpmsg-tty-raw.-1.1: new channel: 0x401 -> 0x1 : ttyRPMSG1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-06 11:33 ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support nikita.shubin
@ 2020-04-06 11:33   ` nikita.shubin
  2020-04-14 16:45     ` Mathieu Poirier
  2020-04-06 11:33   ` [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support nikita.shubin
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 29+ messages in thread
From: nikita.shubin @ 2020-04-06 11:33 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

In case elf file interrupt vector is not supposed to be at OCRAM_S,
it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
firmware.

Otherwise firmware located anywhere besides OCRAM_S won't boot.

The firmware must set stack poiner as first instruction:

Reset_Handler:
    ldr   sp, = __stack      /* set stack pointer */

Signed-off-by: Nikita Shubin <NShubin@topcon.com>
---
 drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 3e72b6f38d4b..bebc58d0f711 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -45,6 +45,8 @@
 
 #define IMX7D_RPROC_MEM_MAX		8
 
+#define IMX_BOOT_PC			0x4
+
 /**
  * struct imx_rproc_mem - slim internal memory structure
  * @cpu_addr: MPU virtual address of the memory region
@@ -85,6 +87,7 @@ struct imx_rproc {
 	const struct imx_rproc_dcfg	*dcfg;
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
+	void __iomem			*bootreg;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
@@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc *rproc)
 	struct device *dev = priv->dev;
 	int ret;
 
+	/* write entry point to program counter */
+	writel(rproc->bootaddr, priv->bootreg);
+
 	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
 				 dcfg->src_mask, dcfg->src_start);
 	if (ret)
 		dev_err(dev, "Failed to enable M4!\n");
 
+	dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
+
 	return ret;
 }
 
@@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
 	if (ret)
 		dev_err(dev, "Failed to stop M4!\n");
 
+	/* clear entry points */
+	writel(0, priv->bootreg);
+
 	return ret;
 }
 
@@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
-	.da_to_va       = imx_rproc_da_to_va,
+	.da_to_va	= imx_rproc_da_to_va,
+	.get_boot_addr	= rproc_elf_get_boot_addr,
 };
 
 static int imx_rproc_addr_init(struct imx_rproc *priv,
@@ -360,6 +372,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_rproc;
 	}
 
+	priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC, sizeof(u32));
+
 	/*
 	 * clk for M4 block including memory. Should be
 	 * enabled before .start for FW transfer.
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support
  2020-04-06 11:33 ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support nikita.shubin
  2020-04-06 11:33   ` [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start nikita.shubin
@ 2020-04-06 11:33   ` nikita.shubin
  2020-04-07  1:07     ` kbuild test robot
                       ` (2 more replies)
  2020-04-06 11:33   ` [PATCH v2 3/3] remoteproc: imx_rproc: memory regions nikita.shubin
  2020-04-15  2:42   ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support Peng Fan
  3 siblings, 3 replies; 29+ messages in thread
From: nikita.shubin @ 2020-04-06 11:33 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-kernel, linux-arm-kernel

Add support for mailboxes to imx_rproc

Signed-off-by: Nikita Shubin <NShubin@topcon.com>
---
 drivers/remoteproc/Kconfig     |   2 +
 drivers/remoteproc/imx_rproc.c | 142 ++++++++++++++++++++++++++++++++-
 2 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 94afdde4bc9f..02d23a54c9cf 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -17,6 +17,8 @@ if REMOTEPROC
 config IMX_REMOTEPROC
 	tristate "IMX6/7 remoteproc support"
 	depends on ARCH_MXC
+	select MAILBOX
+	select IMX_MBOX
 	help
 	  Say y here to support iMX's remote processors (Cortex M4
 	  on iMX7D) via the remote processor framework.
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index bebc58d0f711..d2bede4ccb70 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -14,6 +14,9 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
+#include <linux/mailbox_client.h>
+
+#include "remoteproc_internal.h"
 
 #define IMX7D_SRC_SCR			0x0C
 #define IMX7D_ENABLE_M4			BIT(3)
@@ -47,6 +50,12 @@
 
 #define IMX_BOOT_PC			0x4
 
+#define IMX_MBOX_NB_VQ			2
+#define IMX_MBOX_NB_MBX		2
+
+#define IMX_MBX_VQ0		"vq0"
+#define IMX_MBX_VQ1		"vq1"
+
 /**
  * struct imx_rproc_mem - slim internal memory structure
  * @cpu_addr: MPU virtual address of the memory region
@@ -80,6 +89,14 @@ struct imx_rproc_dcfg {
 	size_t				att_size;
 };
 
+struct imx_mbox {
+	const unsigned char name[10];
+	struct mbox_chan *chan;
+	struct mbox_client client;
+	struct work_struct vq_work;
+	int vq_id;
+};
+
 struct imx_rproc {
 	struct device			*dev;
 	struct regmap			*regmap;
@@ -88,6 +105,8 @@ struct imx_rproc {
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
 	void __iomem			*bootreg;
+	struct imx_mbox mb[IMX_MBOX_NB_MBX];
+	struct workqueue_struct *workqueue;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
@@ -251,10 +270,118 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 	return va;
 }
 
+static void imx_rproc_mb_vq_work(struct work_struct *work)
+{
+	struct imx_mbox *mb = container_of(work, struct imx_mbox, vq_work);
+	struct rproc *rproc = dev_get_drvdata(mb->client.dev);
+
+	if (rproc_vq_interrupt(rproc, mb->vq_id) == IRQ_NONE)
+		dev_dbg(&rproc->dev, "no message found in vq%d\n", mb->vq_id);
+}
+
+static void imx_rproc_mb_callback(struct mbox_client *cl, void *data)
+{
+	struct rproc *rproc = dev_get_drvdata(cl->dev);
+	struct imx_mbox *mb = container_of(cl, struct imx_mbox, client);
+	struct imx_rproc *ddata = rproc->priv;
+
+	queue_work(ddata->workqueue, &mb->vq_work);
+}
+
+static const struct imx_mbox imx_rproc_mbox[IMX_MBOX_NB_MBX] = {
+	{
+		.name = IMX_MBX_VQ0,
+		.vq_id = 0,
+		.client = {
+			.rx_callback = imx_rproc_mb_callback,
+			.tx_block = false,
+		},
+	},
+	{
+		.name = IMX_MBX_VQ1,
+		.vq_id = 1,
+		.client = {
+			.rx_callback = imx_rproc_mb_callback,
+			.tx_block = false,
+		},
+	},
+};
+
+static void imx_rproc_request_mbox(struct rproc *rproc)
+{
+	struct imx_rproc *ddata = rproc->priv;
+	struct device *dev = &rproc->dev;
+	unsigned int i;
+	const unsigned char *name;
+	struct mbox_client *cl;
+
+	/* Initialise mailbox structure table */
+	memcpy(ddata->mb, imx_rproc_mbox, sizeof(imx_rproc_mbox));
+
+	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
+		name = ddata->mb[i].name;
+
+		cl = &ddata->mb[i].client;
+		cl->dev = dev->parent;
+
+		ddata->mb[i].chan = mbox_request_channel_byname(cl, name);
+
+		dev_dbg(dev, "%s: name=%s, idx=%u\n",
+			__func__, name, ddata->mb[i].vq_id);
+
+		if (IS_ERR(ddata->mb[i].chan)) {
+			dev_warn(dev, "cannot get %s mbox\n", name);
+			ddata->mb[i].chan = NULL;
+		}
+
+		if (ddata->mb[i].vq_id >= 0)
+			INIT_WORK(&ddata->mb[i].vq_work, imx_rproc_mb_vq_work);
+	}
+}
+
+static void imx_rproc_free_mbox(struct rproc *rproc)
+{
+	struct imx_rproc *ddata = rproc->priv;
+	unsigned int i;
+
+	dev_dbg(&rproc->dev, "%s: %d boxes\n",
+		__func__, ARRAY_SIZE(ddata->mb));
+
+	for (i = 0; i < ARRAY_SIZE(ddata->mb); i++) {
+		if (ddata->mb[i].chan)
+			mbox_free_channel(ddata->mb[i].chan);
+		ddata->mb[i].chan = NULL;
+	}
+}
+
+static void imx_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct imx_rproc *ddata = rproc->priv;
+	unsigned int i;
+	int err;
+
+	if (WARN_ON(vqid >= IMX_MBOX_NB_VQ))
+		return;
+
+	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
+		if (vqid != ddata->mb[i].vq_id)
+			continue;
+		if (!ddata->mb[i].chan)
+			return;
+		dev_dbg(&rproc->dev, "sending message : vqid = %d\n", vqid);
+		err = mbox_send_message(ddata->mb[i].chan, &vqid);
+		if (err < 0)
+			dev_err(&rproc->dev, "%s: failed (%s, err:%d)\n",
+					__func__, ddata->mb[i].name, err);
+			return;
+	}
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
 	.da_to_va	= imx_rproc_da_to_va,
+	.kick		= imx_rproc_kick,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
 };
 
@@ -384,14 +511,26 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_rproc;
 	}
 
+	priv->workqueue = create_workqueue(dev_name(dev));
+	if (!priv->workqueue) {
+		dev_err(dev, "cannot create workqueue\n");
+		ret = -ENOMEM;
+		goto err_put_clk;
+	}
+
+	imx_rproc_request_mbox(rproc);
+
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
-		goto err_put_clk;
+		goto err_free_mb;
 	}
 
 	return 0;
 
+err_free_mb:
+	imx_rproc_free_mbox(rproc);
+	destroy_workqueue(priv->workqueue);
 err_put_clk:
 	clk_disable_unprepare(priv->clk);
 err_put_rproc:
@@ -407,6 +546,7 @@ static int imx_rproc_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);
+	imx_rproc_free_mbox(rproc);
 	rproc_free(rproc);
 
 	return 0;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v2 3/3] remoteproc: imx_rproc: memory regions
  2020-04-06 11:33 ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support nikita.shubin
  2020-04-06 11:33   ` [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start nikita.shubin
  2020-04-06 11:33   ` [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support nikita.shubin
@ 2020-04-06 11:33   ` nikita.shubin
  2020-04-14 17:46     ` Mathieu Poirier
  2020-04-15  2:42   ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support Peng Fan
  3 siblings, 1 reply; 29+ messages in thread
From: nikita.shubin @ 2020-04-06 11:33 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

Add support for carveout memory regions required for vdev vring's and
buffer.

Search in device tree and allocate memory regions like for ocram:

vdev0vring0: vdev0vring0@00920000 {
	compatible = "shared-dma-pool";
        reg = <0x00920000 0x2000>;
        no-map;
};

vdev0vring1: vdev0vring1@00922000 {
	compatible = "shared-dma-pool";
	reg = <0x00922000 0x2000>;
	no-map;
};

vdev0buffer: vdev0buffer@00924000 {
	compatible = "shared-dma-pool";
	reg = <0x00924000 0x4000>;
	no-map;
};

imx7d-cm4 {
	compatible = "fsl,imx7d-cm4";
	memory-region = <&ocram>, <&vdev0vring0>, <&vdev0vring1>, \
		<&vdev0buffer>;
}

vdev0vring0, vdev0vring1, vdev0buffer are required for virtio
functioning.

Signed-off-by: Nikita Shubin <NShubin@topcon.com>
---
 drivers/remoteproc/imx_rproc.c | 119 ++++++++++++++++++++++++++++++++-
 1 file changed, 118 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index d2bede4ccb70..cdcff2bd2867 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
@@ -238,6 +239,29 @@ static int imx_rproc_da_to_sys(struct imx_rproc *priv, u64 da,
 	return -ENOENT;
 }
 
+static int imx_rproc_sys_to_da(struct imx_rproc *priv, u64 sys,
+				int len, u64 *da)
+{
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	int i;
+
+	/* parse address translation table */
+	for (i = 0; i < dcfg->att_size; i++) {
+		const struct imx_rproc_att *att = &dcfg->att[i];
+
+		if (sys >= att->sa && sys + len <= att->sa + att->size) {
+			unsigned int offset = sys - att->sa;
+
+			*da = att->da + offset;
+			return 0;
+		}
+	}
+
+	dev_warn(priv->dev, "Translation failed: sys = 0x%llx len = 0x%x\n",
+			 sys, len);
+	return -ENOENT;
+}
+
 static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 {
 	struct imx_rproc *priv = rproc->priv;
@@ -372,16 +396,109 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
 		err = mbox_send_message(ddata->mb[i].chan, &vqid);
 		if (err < 0)
 			dev_err(&rproc->dev, "%s: failed (%s, err:%d)\n",
-					__func__, ddata->mb[i].name, err);
+				__func__, ddata->mb[i].name, err);
 			return;
 	}
 }
 
+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: %pa+%x\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: %pa+%x\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_fw(struct rproc *rproc, const struct firmware *fw)
+{
+	struct imx_rproc *priv = rproc->priv;
+	struct device *dev = rproc->dev.parent;
+	struct device_node *np = dev->of_node;
+	struct of_phandle_iterator it;
+	struct rproc_mem_entry *mem = 0;
+	struct reserved_mem *rmem;
+	u64 da;
+	int index = 0;
+
+	/* Register associated reserved memory regions */
+	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
+	while (of_phandle_iterator_next(&it) == 0) {
+		rmem = of_reserved_mem_lookup(it.node);
+		if (!rmem) {
+			dev_err(dev, "unable to acquire memory-region\n");
+			return -EINVAL;
+		}
+
+		/*
+		 * Let's assume all data in device tree is from
+		 * CPU A7 point of view then we should translate
+		 * rmem->base into M4 da
+		 */
+		if (imx_rproc_sys_to_da(priv, rmem->base, rmem->size, &da)) {
+			dev_err(dev, "memory region not valid %pa\n",
+				&rmem->base);
+			return -EINVAL;
+		}
+
+		if (strcmp(it.node->name, "vdev0buffer")) {
+			/* Register memory region */
+			mem = rproc_mem_entry_init(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 {
+			mem = rproc_of_resm_mem_entry_init(dev, index,
+							rmem->size,
+							rmem->base,
+							it.node->name);
+		}
+
+		if (!mem)
+			return -ENOMEM;
+
+		rproc_add_carveout(rproc, mem);
+		index++;
+	}
+
+	return rproc_elf_load_rsc_table(rproc, fw);
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
 	.da_to_va	= imx_rproc_da_to_va,
 	.kick		= imx_rproc_kick,
+	.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,
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support
  2020-04-06 11:33   ` [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support nikita.shubin
@ 2020-04-07  1:07     ` kbuild test robot
  2020-04-14 17:20     ` Mathieu Poirier
  2020-04-14 17:36     ` Mathieu Poirier
  2 siblings, 0 replies; 29+ messages in thread
From: kbuild test robot @ 2020-04-07  1:07 UTC (permalink / raw)
  To: nikita.shubin
  Cc: kbuild-all, Ohad Ben-Cohen, Fabio Estevam, Nikita Shubin,
	Sascha Hauer, linux-remoteproc, linux-kernel, Bjorn Andersson,
	NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
	linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 3482 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on v5.6 next-20200406]
[cannot apply to linus/master remoteproc/for-next rpmsg/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/nikita-shubin-maquefel-me/remoteproc-imx_rproc-add-virtio-support/20200406-201717
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f5ae2ea6347a308cfe91f53b53682ce635497d0d
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:331,
                    from include/linux/kernel.h:15,
                    from include/linux/clk.h:13,
                    from drivers/remoteproc/imx_rproc.c:6:
   drivers/remoteproc/imx_rproc.c: In function 'imx_rproc_free_mbox':
>> drivers/remoteproc/imx_rproc.c:347:23: warning: format '%d' expects argument of type 'int', but argument 5 has type 'long unsigned int' [-Wformat=]
     347 |  dev_dbg(&rproc->dev, "%s: %d boxes\n",
         |                       ^~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:125:15: note: in definition of macro '__dynamic_func_call'
     125 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     157 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/device.h:1784:2: note: in expansion of macro 'dynamic_dev_dbg'
    1784 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/device.h:1784:23: note: in expansion of macro 'dev_fmt'
    1784 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
>> drivers/remoteproc/imx_rproc.c:347:2: note: in expansion of macro 'dev_dbg'
     347 |  dev_dbg(&rproc->dev, "%s: %d boxes\n",
         |  ^~~~~~~
   drivers/remoteproc/imx_rproc.c:347:29: note: format string is defined here
     347 |  dev_dbg(&rproc->dev, "%s: %d boxes\n",
         |                            ~^
         |                             |
         |                             int
         |                            %ld

vim +347 drivers/remoteproc/imx_rproc.c

   341	
   342	static void imx_rproc_free_mbox(struct rproc *rproc)
   343	{
   344		struct imx_rproc *ddata = rproc->priv;
   345		unsigned int i;
   346	
 > 347		dev_dbg(&rproc->dev, "%s: %d boxes\n",
   348			__func__, ARRAY_SIZE(ddata->mb));
   349	
   350		for (i = 0; i < ARRAY_SIZE(ddata->mb); i++) {
   351			if (ddata->mb[i].chan)
   352				mbox_free_channel(ddata->mb[i].chan);
   353			ddata->mb[i].chan = NULL;
   354		}
   355	}
   356	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69606 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-06 11:33   ` [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start nikita.shubin
@ 2020-04-14 16:45     ` Mathieu Poirier
  2020-04-17 12:11       ` Nikita Shubin
       [not found]       ` <45761587100993@mail.yandex.ru>
  0 siblings, 2 replies; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-14 16:45 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

Hi Nikita,

On Mon, Apr 06, 2020 at 02:33:08PM +0300, nikita.shubin@maquefel.me wrote:
> In case elf file interrupt vector is not supposed to be at OCRAM_S,
> it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
> firmware.
> 
> Otherwise firmware located anywhere besides OCRAM_S won't boot.
> 
> The firmware must set stack poiner as first instruction:
> 
> Reset_Handler:
>     ldr   sp, = __stack      /* set stack pointer */
> 
> Signed-off-by: Nikita Shubin <NShubin@topcon.com>

The address in the SoB has to match what is found in the "From:" field of the
email header.  Checkpatch is complaining about that, something I would have
expected to be fixed before sending this set out.

> ---
>  drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 3e72b6f38d4b..bebc58d0f711 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -45,6 +45,8 @@
>  
>  #define IMX7D_RPROC_MEM_MAX		8
>  
> +#define IMX_BOOT_PC			0x4
> +
>  /**
>   * struct imx_rproc_mem - slim internal memory structure
>   * @cpu_addr: MPU virtual address of the memory region
> @@ -85,6 +87,7 @@ struct imx_rproc {
>  	const struct imx_rproc_dcfg	*dcfg;
>  	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
>  	struct clk			*clk;
> +	void __iomem			*bootreg;
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc *rproc)
>  	struct device *dev = priv->dev;
>  	int ret;
>  
> +	/* write entry point to program counter */
> +	writel(rproc->bootaddr, priv->bootreg);

What happens on all the other IMX systems where this fix is not needed?  Will
they continue to work properly?   

> +
>  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
>  				 dcfg->src_mask, dcfg->src_start);
>  	if (ret)
>  		dev_err(dev, "Failed to enable M4!\n");
>  
> +	dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
> +
>  	return ret;
>  }
>  
> @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
>  	if (ret)
>  		dev_err(dev, "Failed to stop M4!\n");
>  
> +	/* clear entry points */
> +	writel(0, priv->bootreg);
> +
>  	return ret;
>  }
>  
> @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
> -	.da_to_va       = imx_rproc_da_to_va,
> +	.da_to_va	= imx_rproc_da_to_va,
> +	.get_boot_addr	= rproc_elf_get_boot_addr,

How is this useful?  Sure it will set rproc->bootaddr in rproc_fw_boot() but
what good does that do when it is invariably set again in imx_rproc_start() ? 

>  };
>  
>  static int imx_rproc_addr_init(struct imx_rproc *priv,
> @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  		goto err_put_rproc;
>  	}
>  
> +	priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC, sizeof(u32));
> +
>  	/*
>  	 * clk for M4 block including memory. Should be
>  	 * enabled before .start for FW transfer.
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support
  2020-04-06 11:33   ` [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support nikita.shubin
  2020-04-07  1:07     ` kbuild test robot
@ 2020-04-14 17:20     ` Mathieu Poirier
  2020-04-17  8:37       ` Nikita Shubin
  2020-04-14 17:36     ` Mathieu Poirier
  2 siblings, 1 reply; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-14 17:20 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-kernel, linux-arm-kernel

On Mon, Apr 06, 2020 at 02:33:09PM +0300, nikita.shubin@maquefel.me wrote:
> Add support for mailboxes to imx_rproc
> 
> Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> ---
>  drivers/remoteproc/Kconfig     |   2 +
>  drivers/remoteproc/imx_rproc.c | 142 ++++++++++++++++++++++++++++++++-
>  2 files changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 94afdde4bc9f..02d23a54c9cf 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -17,6 +17,8 @@ if REMOTEPROC
>  config IMX_REMOTEPROC
>  	tristate "IMX6/7 remoteproc support"
>  	depends on ARCH_MXC
> +	select MAILBOX
> +	select IMX_MBOX
>  	help
>  	  Say y here to support iMX's remote processors (Cortex M4
>  	  on iMX7D) via the remote processor framework.
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index bebc58d0f711..d2bede4ccb70 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -14,6 +14,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
> +#include <linux/mailbox_client.h>
> +
> +#include "remoteproc_internal.h"
>  
>  #define IMX7D_SRC_SCR			0x0C
>  #define IMX7D_ENABLE_M4			BIT(3)
> @@ -47,6 +50,12 @@
>  
>  #define IMX_BOOT_PC			0x4
>  
> +#define IMX_MBOX_NB_VQ			2
> +#define IMX_MBOX_NB_MBX		2

Please align this.

> +
> +#define IMX_MBX_VQ0		"vq0"
> +#define IMX_MBX_VQ1		"vq1"
> +
>  /**
>   * struct imx_rproc_mem - slim internal memory structure
>   * @cpu_addr: MPU virtual address of the memory region
> @@ -80,6 +89,14 @@ struct imx_rproc_dcfg {
>  	size_t				att_size;
>  };
>  
> +struct imx_mbox {
> +	const unsigned char name[10];
> +	struct mbox_chan *chan;
> +	struct mbox_client client;
> +	struct work_struct vq_work;
> +	int vq_id;
> +};
> +
>  struct imx_rproc {
>  	struct device			*dev;
>  	struct regmap			*regmap;
> @@ -88,6 +105,8 @@ struct imx_rproc {
>  	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
>  	struct clk			*clk;
>  	void __iomem			*bootreg;
> +	struct imx_mbox mb[IMX_MBOX_NB_MBX];
> +	struct workqueue_struct *workqueue;
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> @@ -251,10 +270,118 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>  	return va;
>  }
>  
> +static void imx_rproc_mb_vq_work(struct work_struct *work)
> +{
> +	struct imx_mbox *mb = container_of(work, struct imx_mbox, vq_work);
> +	struct rproc *rproc = dev_get_drvdata(mb->client.dev);
> +
> +	if (rproc_vq_interrupt(rproc, mb->vq_id) == IRQ_NONE)
> +		dev_dbg(&rproc->dev, "no message found in vq%d\n", mb->vq_id);
> +}
> +
> +static void imx_rproc_mb_callback(struct mbox_client *cl, void *data)
> +{
> +	struct rproc *rproc = dev_get_drvdata(cl->dev);
> +	struct imx_mbox *mb = container_of(cl, struct imx_mbox, client);
> +	struct imx_rproc *ddata = rproc->priv;
> +
> +	queue_work(ddata->workqueue, &mb->vq_work);
> +}
> +
> +static const struct imx_mbox imx_rproc_mbox[IMX_MBOX_NB_MBX] = {
> +	{
> +		.name = IMX_MBX_VQ0,
> +		.vq_id = 0,
> +		.client = {
> +			.rx_callback = imx_rproc_mb_callback,
> +			.tx_block = false,
> +		},
> +	},
> +	{
> +		.name = IMX_MBX_VQ1,
> +		.vq_id = 1,
> +		.client = {
> +			.rx_callback = imx_rproc_mb_callback,
> +			.tx_block = false,
> +		},
> +	},
> +};
> +
> +static void imx_rproc_request_mbox(struct rproc *rproc)
> +{
> +	struct imx_rproc *ddata = rproc->priv;
> +	struct device *dev = &rproc->dev;
> +	unsigned int i;
> +	const unsigned char *name;
> +	struct mbox_client *cl;
> +
> +	/* Initialise mailbox structure table */
> +	memcpy(ddata->mb, imx_rproc_mbox, sizeof(imx_rproc_mbox));
> +
> +	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> +		name = ddata->mb[i].name;
> +
> +		cl = &ddata->mb[i].client;
> +		cl->dev = dev->parent;
> +
> +		ddata->mb[i].chan = mbox_request_channel_byname(cl, name);
> +
> +		dev_dbg(dev, "%s: name=%s, idx=%u\n",
> +			__func__, name, ddata->mb[i].vq_id);
> +
> +		if (IS_ERR(ddata->mb[i].chan)) {
> +			dev_warn(dev, "cannot get %s mbox\n", name);
> +			ddata->mb[i].chan = NULL;

If the mailbox isn't ready this driver will fail without a chance of recovery.
Since most of the code in this patch is a carbon copy of the implementation
found in stm32_proc.c, I suggest you do the same as they did in
stm32_rproc_request_mbox() and privision for cases where requesting a channel
returns -EPROBE_DEFER.

> +		}
> +
> +		if (ddata->mb[i].vq_id >= 0)
> +			INIT_WORK(&ddata->mb[i].vq_work, imx_rproc_mb_vq_work);
> +	}
> +}
> +
> +static void imx_rproc_free_mbox(struct rproc *rproc)
> +{
> +	struct imx_rproc *ddata = rproc->priv;
> +	unsigned int i;
> +
> +	dev_dbg(&rproc->dev, "%s: %d boxes\n",
> +		__func__, ARRAY_SIZE(ddata->mb));
> +
> +	for (i = 0; i < ARRAY_SIZE(ddata->mb); i++) {
> +		if (ddata->mb[i].chan)
> +			mbox_free_channel(ddata->mb[i].chan);
> +		ddata->mb[i].chan = NULL;
> +	}
> +}
> +
> +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct imx_rproc *ddata = rproc->priv;
> +	unsigned int i;
> +	int err;
> +
> +	if (WARN_ON(vqid >= IMX_MBOX_NB_VQ))
> +		return;
> +
> +	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> +		if (vqid != ddata->mb[i].vq_id)
> +			continue;
> +		if (!ddata->mb[i].chan)
> +			return;
> +		dev_dbg(&rproc->dev, "sending message : vqid = %d\n", vqid);
> +		err = mbox_send_message(ddata->mb[i].chan, &vqid);
> +		if (err < 0)
> +			dev_err(&rproc->dev, "%s: failed (%s, err:%d)\n",
> +					__func__, ddata->mb[i].name, err);
> +			return;
> +	}
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
>  	.da_to_va	= imx_rproc_da_to_va,
> +	.kick		= imx_rproc_kick,
>  	.get_boot_addr	= rproc_elf_get_boot_addr,
>  };
>  
> @@ -384,14 +511,26 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  		goto err_put_rproc;
>  	}
>  
> +	priv->workqueue = create_workqueue(dev_name(dev));
> +	if (!priv->workqueue) {
> +		dev_err(dev, "cannot create workqueue\n");
> +		ret = -ENOMEM;
> +		goto err_put_clk;
> +	}
> +
> +	imx_rproc_request_mbox(rproc);
> +
>  	ret = rproc_add(rproc);
>  	if (ret) {
>  		dev_err(dev, "rproc_add failed\n");
> -		goto err_put_clk;
> +		goto err_free_mb;
>  	}
>  
>  	return 0;
>  
> +err_free_mb:
> +	imx_rproc_free_mbox(rproc);
> +	destroy_workqueue(priv->workqueue);
>  err_put_clk:
>  	clk_disable_unprepare(priv->clk);
>  err_put_rproc:
> @@ -407,6 +546,7 @@ static int imx_rproc_remove(struct platform_device *pdev)
>  
>  	clk_disable_unprepare(priv->clk);
>  	rproc_del(rproc);
> +	imx_rproc_free_mbox(rproc);

I have no issues with people reusing code already found in the kernel - in fact
I encourage it because it makes reviewing patches much easier.  On the flip side
you have to give credit where it is due.  Here adding a line in the changelog
that mentions where you took your inspiration from will be much appreciated.

Thanks,
Mathieu

>  	rproc_free(rproc);
>  
>  	return 0;
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support
  2020-04-06 11:33   ` [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support nikita.shubin
  2020-04-07  1:07     ` kbuild test robot
  2020-04-14 17:20     ` Mathieu Poirier
@ 2020-04-14 17:36     ` Mathieu Poirier
  2 siblings, 0 replies; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-14 17:36 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-kernel, linux-arm-kernel

On Mon, Apr 06, 2020 at 02:33:09PM +0300, nikita.shubin@maquefel.me wrote:
> Add support for mailboxes to imx_rproc
> 
> Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> ---
>  drivers/remoteproc/Kconfig     |   2 +
>  drivers/remoteproc/imx_rproc.c | 142 ++++++++++++++++++++++++++++++++-
>  2 files changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 94afdde4bc9f..02d23a54c9cf 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -17,6 +17,8 @@ if REMOTEPROC
>  config IMX_REMOTEPROC
>  	tristate "IMX6/7 remoteproc support"
>  	depends on ARCH_MXC
> +	select MAILBOX
> +	select IMX_MBOX
>  	help
>  	  Say y here to support iMX's remote processors (Cortex M4
>  	  on iMX7D) via the remote processor framework.
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index bebc58d0f711..d2bede4ccb70 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -14,6 +14,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
> +#include <linux/mailbox_client.h>
> +
> +#include "remoteproc_internal.h"
>  
>  #define IMX7D_SRC_SCR			0x0C
>  #define IMX7D_ENABLE_M4			BIT(3)
> @@ -47,6 +50,12 @@
>  
>  #define IMX_BOOT_PC			0x4
>  
> +#define IMX_MBOX_NB_VQ			2
> +#define IMX_MBOX_NB_MBX		2
> +
> +#define IMX_MBX_VQ0		"vq0"
> +#define IMX_MBX_VQ1		"vq1"
> +
>  /**
>   * struct imx_rproc_mem - slim internal memory structure
>   * @cpu_addr: MPU virtual address of the memory region
> @@ -80,6 +89,14 @@ struct imx_rproc_dcfg {
>  	size_t				att_size;
>  };
>  
> +struct imx_mbox {
> +	const unsigned char name[10];
> +	struct mbox_chan *chan;
> +	struct mbox_client client;
> +	struct work_struct vq_work;
> +	int vq_id;
> +};
> +
>  struct imx_rproc {
>  	struct device			*dev;
>  	struct regmap			*regmap;
> @@ -88,6 +105,8 @@ struct imx_rproc {
>  	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
>  	struct clk			*clk;
>  	void __iomem			*bootreg;
> +	struct imx_mbox mb[IMX_MBOX_NB_MBX];
> +	struct workqueue_struct *workqueue;
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> @@ -251,10 +270,118 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>  	return va;
>  }
>  
> +static void imx_rproc_mb_vq_work(struct work_struct *work)
> +{
> +	struct imx_mbox *mb = container_of(work, struct imx_mbox, vq_work);
> +	struct rproc *rproc = dev_get_drvdata(mb->client.dev);
> +
> +	if (rproc_vq_interrupt(rproc, mb->vq_id) == IRQ_NONE)
> +		dev_dbg(&rproc->dev, "no message found in vq%d\n", mb->vq_id);
> +}
> +
> +static void imx_rproc_mb_callback(struct mbox_client *cl, void *data)
> +{
> +	struct rproc *rproc = dev_get_drvdata(cl->dev);
> +	struct imx_mbox *mb = container_of(cl, struct imx_mbox, client);
> +	struct imx_rproc *ddata = rproc->priv;
> +
> +	queue_work(ddata->workqueue, &mb->vq_work);
> +}
> +
> +static const struct imx_mbox imx_rproc_mbox[IMX_MBOX_NB_MBX] = {
> +	{
> +		.name = IMX_MBX_VQ0,
> +		.vq_id = 0,
> +		.client = {
> +			.rx_callback = imx_rproc_mb_callback,
> +			.tx_block = false,
> +		},
> +	},
> +	{
> +		.name = IMX_MBX_VQ1,
> +		.vq_id = 1,
> +		.client = {
> +			.rx_callback = imx_rproc_mb_callback,
> +			.tx_block = false,
> +		},
> +	},
> +};
> +
> +static void imx_rproc_request_mbox(struct rproc *rproc)
> +{
> +	struct imx_rproc *ddata = rproc->priv;
> +	struct device *dev = &rproc->dev;
> +	unsigned int i;
> +	const unsigned char *name;
> +	struct mbox_client *cl;
> +
> +	/* Initialise mailbox structure table */
> +	memcpy(ddata->mb, imx_rproc_mbox, sizeof(imx_rproc_mbox));
> +
> +	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> +		name = ddata->mb[i].name;
> +
> +		cl = &ddata->mb[i].client;
> +		cl->dev = dev->parent;
> +
> +		ddata->mb[i].chan = mbox_request_channel_byname(cl, name);

I forgot... You will also need to update the bindings document (imx-rproc.txt).

> +
> +		dev_dbg(dev, "%s: name=%s, idx=%u\n",
> +			__func__, name, ddata->mb[i].vq_id);
> +
> +		if (IS_ERR(ddata->mb[i].chan)) {
> +			dev_warn(dev, "cannot get %s mbox\n", name);
> +			ddata->mb[i].chan = NULL;
> +		}
> +
> +		if (ddata->mb[i].vq_id >= 0)
> +			INIT_WORK(&ddata->mb[i].vq_work, imx_rproc_mb_vq_work);
> +	}
> +}
> +
> +static void imx_rproc_free_mbox(struct rproc *rproc)
> +{
> +	struct imx_rproc *ddata = rproc->priv;
> +	unsigned int i;
> +
> +	dev_dbg(&rproc->dev, "%s: %d boxes\n",
> +		__func__, ARRAY_SIZE(ddata->mb));
> +
> +	for (i = 0; i < ARRAY_SIZE(ddata->mb); i++) {
> +		if (ddata->mb[i].chan)
> +			mbox_free_channel(ddata->mb[i].chan);
> +		ddata->mb[i].chan = NULL;
> +	}
> +}
> +
> +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct imx_rproc *ddata = rproc->priv;
> +	unsigned int i;
> +	int err;
> +
> +	if (WARN_ON(vqid >= IMX_MBOX_NB_VQ))
> +		return;
> +
> +	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> +		if (vqid != ddata->mb[i].vq_id)
> +			continue;
> +		if (!ddata->mb[i].chan)
> +			return;
> +		dev_dbg(&rproc->dev, "sending message : vqid = %d\n", vqid);
> +		err = mbox_send_message(ddata->mb[i].chan, &vqid);
> +		if (err < 0)
> +			dev_err(&rproc->dev, "%s: failed (%s, err:%d)\n",
> +					__func__, ddata->mb[i].name, err);
> +			return;
> +	}
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
>  	.da_to_va	= imx_rproc_da_to_va,
> +	.kick		= imx_rproc_kick,
>  	.get_boot_addr	= rproc_elf_get_boot_addr,
>  };
>  
> @@ -384,14 +511,26 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  		goto err_put_rproc;
>  	}
>  
> +	priv->workqueue = create_workqueue(dev_name(dev));
> +	if (!priv->workqueue) {
> +		dev_err(dev, "cannot create workqueue\n");
> +		ret = -ENOMEM;
> +		goto err_put_clk;
> +	}
> +
> +	imx_rproc_request_mbox(rproc);
> +
>  	ret = rproc_add(rproc);
>  	if (ret) {
>  		dev_err(dev, "rproc_add failed\n");
> -		goto err_put_clk;
> +		goto err_free_mb;
>  	}
>  
>  	return 0;
>  
> +err_free_mb:
> +	imx_rproc_free_mbox(rproc);
> +	destroy_workqueue(priv->workqueue);
>  err_put_clk:
>  	clk_disable_unprepare(priv->clk);
>  err_put_rproc:
> @@ -407,6 +546,7 @@ static int imx_rproc_remove(struct platform_device *pdev)
>  
>  	clk_disable_unprepare(priv->clk);
>  	rproc_del(rproc);
> +	imx_rproc_free_mbox(rproc);
>  	rproc_free(rproc);
>  
>  	return 0;
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 3/3] remoteproc: imx_rproc: memory regions
  2020-04-06 11:33   ` [PATCH v2 3/3] remoteproc: imx_rproc: memory regions nikita.shubin
@ 2020-04-14 17:46     ` Mathieu Poirier
  0 siblings, 0 replies; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-14 17:46 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

On Mon, Apr 06, 2020 at 02:33:10PM +0300, nikita.shubin@maquefel.me wrote:
> Add support for carveout memory regions required for vdev vring's and
> buffer.
> 
> Search in device tree and allocate memory regions like for ocram:
> 
> vdev0vring0: vdev0vring0@00920000 {
> 	compatible = "shared-dma-pool";
>         reg = <0x00920000 0x2000>;
>         no-map;
> };
> 
> vdev0vring1: vdev0vring1@00922000 {
> 	compatible = "shared-dma-pool";
> 	reg = <0x00922000 0x2000>;
> 	no-map;
> };
> 
> vdev0buffer: vdev0buffer@00924000 {
> 	compatible = "shared-dma-pool";
> 	reg = <0x00924000 0x4000>;
> 	no-map;
> };
> 
> imx7d-cm4 {
> 	compatible = "fsl,imx7d-cm4";
> 	memory-region = <&ocram>, <&vdev0vring0>, <&vdev0vring1>, \
> 		<&vdev0buffer>;
> }
> 
> vdev0vring0, vdev0vring1, vdev0buffer are required for virtio
> functioning.
> 
> Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 119 ++++++++++++++++++++++++++++++++-
>  1 file changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index d2bede4ccb70..cdcff2bd2867 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
> @@ -238,6 +239,29 @@ static int imx_rproc_da_to_sys(struct imx_rproc *priv, u64 da,
>  	return -ENOENT;
>  }
>  
> +static int imx_rproc_sys_to_da(struct imx_rproc *priv, u64 sys,
> +				int len, u64 *da)
> +{
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> +	int i;
> +
> +	/* parse address translation table */
> +	for (i = 0; i < dcfg->att_size; i++) {
> +		const struct imx_rproc_att *att = &dcfg->att[i];
> +
> +		if (sys >= att->sa && sys + len <= att->sa + att->size) {
> +			unsigned int offset = sys - att->sa;
> +
> +			*da = att->da + offset;
> +			return 0;
> +		}
> +	}
> +
> +	dev_warn(priv->dev, "Translation failed: sys = 0x%llx len = 0x%x\n",
> +			 sys, len);
> +	return -ENOENT;
> +}
> +
>  static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>  {
>  	struct imx_rproc *priv = rproc->priv;
> @@ -372,16 +396,109 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
>  		err = mbox_send_message(ddata->mb[i].chan, &vqid);
>  		if (err < 0)
>  			dev_err(&rproc->dev, "%s: failed (%s, err:%d)\n",
> -					__func__, ddata->mb[i].name, err);
> +				__func__, ddata->mb[i].name, err);
>  			return;
>  	}
>  }
>  
> +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: %pa+%x\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: %pa+%x\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_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	struct device *dev = rproc->dev.parent;
> +	struct device_node *np = dev->of_node;
> +	struct of_phandle_iterator it;
> +	struct rproc_mem_entry *mem = 0;
> +	struct reserved_mem *rmem;
> +	u64 da;
> +	int index = 0;
> +
> +	/* Register associated reserved memory regions */
> +	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);

This will likely clash with the parsing done in imx_rproc_addr_init(), and the
parsing in there will also clash with what is done below...  I advise you to
register carvouts in imx_rproc_addr_init() as you parse the rest of the memory
regions.

Thanks,
Mathieu

> +	while (of_phandle_iterator_next(&it) == 0) {
> +		rmem = of_reserved_mem_lookup(it.node);
> +		if (!rmem) {
> +			dev_err(dev, "unable to acquire memory-region\n");
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * Let's assume all data in device tree is from
> +		 * CPU A7 point of view then we should translate
> +		 * rmem->base into M4 da
> +		 */
> +		if (imx_rproc_sys_to_da(priv, rmem->base, rmem->size, &da)) {
> +			dev_err(dev, "memory region not valid %pa\n",
> +				&rmem->base);
> +			return -EINVAL;
> +		}
> +
> +		if (strcmp(it.node->name, "vdev0buffer")) {
> +			/* Register memory region */
> +			mem = rproc_mem_entry_init(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 {
> +			mem = rproc_of_resm_mem_entry_init(dev, index,
> +							rmem->size,
> +							rmem->base,
> +							it.node->name);
> +		}
> +
> +		if (!mem)
> +			return -ENOMEM;
> +
> +		rproc_add_carveout(rproc, mem);
> +		index++;
> +	}
> +
> +	return rproc_elf_load_rsc_table(rproc, fw);
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
>  	.da_to_va	= imx_rproc_da_to_va,
>  	.kick		= imx_rproc_kick,
> +	.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,
>  };
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* RE: [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support
  2020-04-06 11:33 ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support nikita.shubin
                     ` (2 preceding siblings ...)
  2020-04-06 11:33   ` [PATCH v2 3/3] remoteproc: imx_rproc: memory regions nikita.shubin
@ 2020-04-15  2:42   ` Peng Fan
  2020-04-15 16:26     ` Mathieu Poirier
  2020-04-17  8:57     ` Nikita Shubin
  3 siblings, 2 replies; 29+ messages in thread
From: Peng Fan @ 2020-04-15  2:42 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, dl-linux-imx,
	linux-remoteproc, linux-arm-kernel, linux-kernel

> Subject: [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support

Have you ever see https://patchwork.kernel.org/cover/11390477/?

I have been waiting for Mathieu's rproc sync state patch, then
rebase.

Thanks,
Peng.

> 
> This patch set introduces virtio support for imx7d-m4 communication:
> 
> - support booting loaded vim imx-rproc firmware
> - implement .kick method support using mailbox in imx-processor
> - parse vdev0vring0, vdev0vring1, vdev0buffer memory regions required for
> virtio_rpmsg_bus initialization
> 
> Regarding imx7d-m4 boot proccess
> 
> Citing ARM documentation:
> 
> At Reset, Cortex-M3 and Cortex-M4 processors always boot from a vector
> table at address zero.
> 
> "With uninitialized memory at address zero (for example, unprogrammed
> Flash or uninitialized RAM), the processor will read a spurious initial Main
> Stack Pointer value from address zero and a spurious code entry point (Reset
> vector) from address 0x4, possibly containing an illegal instruction set state
> specifier (ESPR.T bit) in bit[0]."
> 
> So to successfully boot m4 coproc we need to write Stack Pointer and
> Program counter, i see no obvious to get Stack Pointer value, so two ways
> exist ethier form a special elf section:
> 
> "
> .loader :
>   {
>     LONG(__StackTop);
>     LONG(Reset_Handler + 1);
>   } > m_start
> "
> 
> and put it at 0x0 address:
> 
> "
> m_start               (RX)  : ORIGIN = 0x00000000, LENGTH =
> 0x00008000
> "
> 
> Or (the way i've chosen) only put Entry Point at 0x04 and set stack as first
> instruction:
> 
> "
> Reset_Handler:
> 	ldr   sp, =__stack      /* set stack pointer */
> "
> 
> Regarding mailboxes and memory regions :
> 
> This code is heavily derived from stm32-rproc (i.e. copy pasted) and this fact
> needs to reflected in commits, please tell me how to emphasize this fact.
> 
> Attaching succesful trace booting m4 (with Add rpmsg tty driver applied) :
> 
> [  143.240616] remoteproc remoteproc0: powering up imx-rproc
> [  143.251768] remoteproc remoteproc0: Booting fw image huginn.elf, size
> 466876 [  143.251786] imx-rproc imx7d-cm4: iommu not present
> [  143.251825] remoteproc remoteproc0: rsc: type 3 [  143.251837]
> remoteproc remoteproc0: vdev rsc: id 7, dfeatures 0x1, cfg len 0, 2 vrings
> [  143.251924] remoteproc remoteproc0: vdev rsc: vring0: da 0xffffffff, qsz
> 16, align 16 [  143.251935] remoteproc remoteproc0: vdev rsc: vring1: da
> 0xffffffff, qsz 16, align 16 [  143.251955] imx-rproc imx7d-cm4: map memory:
> 0x00900000+20000 [  143.251987] imx-rproc imx7d-cm4: map memory:
> 0x00920000+2000 [  143.252003] imx-rproc imx7d-cm4: map memory:
> 0x00922000+2000 [  143.252020] remoteproc remoteproc0: phdr: type 1 da
> 0x20200000 memsz 0x240 filesz 0x240 [  143.252032] remoteproc
> remoteproc0: da = 0x20200000 len = 0x240 va = 0x(ptrval) [  143.252043]
> remoteproc remoteproc0: phdr: type 1 da 0x20200240 memsz 0x5b38 filesz
> 0x5b38 [  143.252053] remoteproc remoteproc0: da = 0x20200240 len =
> 0x5b38 va = 0x(ptrval) [  143.252105] remoteproc remoteproc0: phdr: type
> 1 da 0x20205d78 memsz 0x4b58 filesz 0x758 [  143.252115] remoteproc
> remoteproc0: da = 0x20205d78 len = 0x4b58 va = 0x(ptrval) [  143.252159]
> remoteproc remoteproc0: da = 0x200006cc len = 0x8c va = 0x(ptrval)
> [  143.252176] remoteproc remoteproc0: Started from 0x202002f5
> [  143.252211]  imx7d-cm4#vdev0buffer: assigned reserved memory node
> vdev0buffer@00924000 [  143.252232] virtio virtio0: reset !
> [  143.252241] virtio virtio0: status: 1 [  143.260567] virtio_rpmsg_bus
> virtio0: status: 3 [  143.260598] remoteproc remoteproc0: vring0: va
> c083c000 qsz 16 notifyid 0 [  143.260614] remoteproc remoteproc0: vring1:
> va c0872000 qsz 16 notifyid 1 [  143.260651] virtio_rpmsg_bus virtio0:
> buffers: va c0894000, dma 0x00924000 [  143.260666] Added buffer head 0
> to (ptrval) [  143.260674] Added buffer head 1 to (ptrval) [  143.260680]
> Added buffer head 2 to (ptrval) [  143.260686] Added buffer head 3 to (ptrval)
> [  143.260692] Added buffer head 4 to (ptrval) [  143.260697] Added buffer
> head 5 to (ptrval) [  143.260703] Added buffer head 6 to (ptrval)
> [  143.260709] Added buffer head 7 to (ptrval) [  143.260715] Added buffer
> head 8 to (ptrval) [  143.260721] Added buffer head 9 to (ptrval)
> [  143.260727] Added buffer head 10 to (ptrval) [  143.260733] Added
> buffer head 11 to (ptrval) [  143.260738] Added buffer head 12 to (ptrval)
> [  143.260744] Added buffer head 13 to (ptrval) [  143.260750] Added
> buffer head 14 to (ptrval) [  143.260756] Added buffer head 15 to (ptrval)
> [  143.260771] virtio_rpmsg_bus virtio0: status: 7 [  143.260779]
> remoteproc remoteproc0: kicking vq index: 0 [  143.260788] remoteproc
> remoteproc0: sending message : vqid = 0 [  143.260802] imx_mu
> 30aa0000.mailbox: Send data on wrong channel type: 1 [  143.260810]
> virtio_rpmsg_bus virtio0: rpmsg host is online [  143.261680]
> imx7d-cm4#vdev0buffer: registered virtio0 (type 7) [  143.261694]
> remoteproc remoteproc0: remote processor imx-rproc is now up
> [  143.354880] remoteproc remoteproc0: vq index 0 is interrupted
> [  143.354895] virtqueue callback for (ptrval) ((ptrval)) [  143.354912]
> virtio_rpmsg_bus virtio0: From: 0x0, To: 0x35, Len: 40, Flags: 0, Reserved: 0
> [  143.354924] rpmsg_virtio RX: 00 00 00 00 35 00 00 00 00 00 00 00 28 00
> 00 00  ....5.......(...
> [  143.354932] rpmsg_virtio RX: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00
> 00 00  rpmsg-tty-raw...
> [  143.354939] rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00  ................
> [  143.354945] rpmsg_virtio RX: 00 00 00 00 00 00 00
> 00                          ........
> [  143.354956] NS announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77
> 00 00 00  rpmsg-tty-raw...
> [  143.354963] NS announcement: 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00  ................
> [  143.354969] NS announcement: 00 00 00 00 00 00 00
> 00                          ........
> [  143.354980] virtio_rpmsg_bus virtio0: creating channel rpmsg-tty-raw
> addr 0x0 [  143.356584] rpmsg_tty virtio0.rpmsg-tty-raw.-1.0: new channel:
> 0x400 -> 0x0 : ttyRPMSG0 [  143.356651] Added buffer head 0 to (ptrval)
> [  143.356658] No more buffers in queue [  143.356667] virtio_rpmsg_bus
> virtio0: Received 1 messages [  143.404302] remoteproc remoteproc0: vq
> index 0 is interrupted [  143.404319] virtqueue callback for (ptrval) ((ptrval))
> [  143.404337] virtio_rpmsg_bus virtio0: From: 0x1, To: 0x35, Len: 40, Flags:
> 0, Reserved: 0 [  143.404350] rpmsg_virtio RX: 01 00 00 00 35 00 00 00 00
> 00 00 00 28 00 00 00  ....5.......(...
> [  143.404391] rpmsg_virtio RX: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00
> 00 00  rpmsg-tty-raw...
> [  143.404399] rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00  ................
> [  143.404405] rpmsg_virtio RX: 01 00 00 00 00 00 00
> 00                          ........
> [  143.404417] NS announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77
> 00 00 00  rpmsg-tty-raw...
> [  143.404424] NS announcement: 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00  ................
> [  143.404430] NS announcement: 01 00 00 00 00 00 00
> 00                          ........
> [  143.404441] virtio_rpmsg_bus virtio0: creating channel rpmsg-tty-raw
> addr 0x1 [  143.411114] rpmsg_tty virtio0.rpmsg-tty-raw.-1.1: new channel:
> 0x401 -> 0x1 : ttyRPMSG1

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support
  2020-04-15  2:42   ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support Peng Fan
@ 2020-04-15 16:26     ` Mathieu Poirier
  2020-04-17  8:57     ` Nikita Shubin
  1 sibling, 0 replies; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-15 16:26 UTC (permalink / raw)
  To: Peng Fan
  Cc: nikita.shubin, Ohad Ben-Cohen, Shawn Guo, Sascha Hauer,
	linux-remoteproc, linux-kernel, Bjorn Andersson, dl-linux-imx,
	Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel

On Tue, 14 Apr 2020 at 20:42, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support
>
> Have you ever see https://patchwork.kernel.org/cover/11390477/?
>
> I have been waiting for Mathieu's rproc sync state patch, then
> rebase.

I have already sent out 2 revisions of the MCU synchronisation
patchset, the latest here [1].  A new iteration should be out by the
end of the week or early next week.  When that happens, I would really
appreciate it if you could take a look and provide comments.

Thanks,
Mathieu

[1].https://patchwork.kernel.org/project/linux-remoteproc/list/?series=261069


>
> Thanks,
> Peng.
>
> >
> > This patch set introduces virtio support for imx7d-m4 communication:
> >
> > - support booting loaded vim imx-rproc firmware
> > - implement .kick method support using mailbox in imx-processor
> > - parse vdev0vring0, vdev0vring1, vdev0buffer memory regions required for
> > virtio_rpmsg_bus initialization
> >
> > Regarding imx7d-m4 boot proccess
> >
> > Citing ARM documentation:
> >
> > At Reset, Cortex-M3 and Cortex-M4 processors always boot from a vector
> > table at address zero.
> >
> > "With uninitialized memory at address zero (for example, unprogrammed
> > Flash or uninitialized RAM), the processor will read a spurious initial Main
> > Stack Pointer value from address zero and a spurious code entry point (Reset
> > vector) from address 0x4, possibly containing an illegal instruction set state
> > specifier (ESPR.T bit) in bit[0]."
> >
> > So to successfully boot m4 coproc we need to write Stack Pointer and
> > Program counter, i see no obvious to get Stack Pointer value, so two ways
> > exist ethier form a special elf section:
> >
> > "
> > .loader :
> >   {
> >     LONG(__StackTop);
> >     LONG(Reset_Handler + 1);
> >   } > m_start
> > "
> >
> > and put it at 0x0 address:
> >
> > "
> > m_start               (RX)  : ORIGIN = 0x00000000, LENGTH =
> > 0x00008000
> > "
> >
> > Or (the way i've chosen) only put Entry Point at 0x04 and set stack as first
> > instruction:
> >
> > "
> > Reset_Handler:
> >       ldr   sp, =__stack      /* set stack pointer */
> > "
> >
> > Regarding mailboxes and memory regions :
> >
> > This code is heavily derived from stm32-rproc (i.e. copy pasted) and this fact
> > needs to reflected in commits, please tell me how to emphasize this fact.
> >
> > Attaching succesful trace booting m4 (with Add rpmsg tty driver applied) :
> >
> > [  143.240616] remoteproc remoteproc0: powering up imx-rproc
> > [  143.251768] remoteproc remoteproc0: Booting fw image huginn.elf, size
> > 466876 [  143.251786] imx-rproc imx7d-cm4: iommu not present
> > [  143.251825] remoteproc remoteproc0: rsc: type 3 [  143.251837]
> > remoteproc remoteproc0: vdev rsc: id 7, dfeatures 0x1, cfg len 0, 2 vrings
> > [  143.251924] remoteproc remoteproc0: vdev rsc: vring0: da 0xffffffff, qsz
> > 16, align 16 [  143.251935] remoteproc remoteproc0: vdev rsc: vring1: da
> > 0xffffffff, qsz 16, align 16 [  143.251955] imx-rproc imx7d-cm4: map memory:
> > 0x00900000+20000 [  143.251987] imx-rproc imx7d-cm4: map memory:
> > 0x00920000+2000 [  143.252003] imx-rproc imx7d-cm4: map memory:
> > 0x00922000+2000 [  143.252020] remoteproc remoteproc0: phdr: type 1 da
> > 0x20200000 memsz 0x240 filesz 0x240 [  143.252032] remoteproc
> > remoteproc0: da = 0x20200000 len = 0x240 va = 0x(ptrval) [  143.252043]
> > remoteproc remoteproc0: phdr: type 1 da 0x20200240 memsz 0x5b38 filesz
> > 0x5b38 [  143.252053] remoteproc remoteproc0: da = 0x20200240 len =
> > 0x5b38 va = 0x(ptrval) [  143.252105] remoteproc remoteproc0: phdr: type
> > 1 da 0x20205d78 memsz 0x4b58 filesz 0x758 [  143.252115] remoteproc
> > remoteproc0: da = 0x20205d78 len = 0x4b58 va = 0x(ptrval) [  143.252159]
> > remoteproc remoteproc0: da = 0x200006cc len = 0x8c va = 0x(ptrval)
> > [  143.252176] remoteproc remoteproc0: Started from 0x202002f5
> > [  143.252211]  imx7d-cm4#vdev0buffer: assigned reserved memory node
> > vdev0buffer@00924000 [  143.252232] virtio virtio0: reset !
> > [  143.252241] virtio virtio0: status: 1 [  143.260567] virtio_rpmsg_bus
> > virtio0: status: 3 [  143.260598] remoteproc remoteproc0: vring0: va
> > c083c000 qsz 16 notifyid 0 [  143.260614] remoteproc remoteproc0: vring1:
> > va c0872000 qsz 16 notifyid 1 [  143.260651] virtio_rpmsg_bus virtio0:
> > buffers: va c0894000, dma 0x00924000 [  143.260666] Added buffer head 0
> > to (ptrval) [  143.260674] Added buffer head 1 to (ptrval) [  143.260680]
> > Added buffer head 2 to (ptrval) [  143.260686] Added buffer head 3 to (ptrval)
> > [  143.260692] Added buffer head 4 to (ptrval) [  143.260697] Added buffer
> > head 5 to (ptrval) [  143.260703] Added buffer head 6 to (ptrval)
> > [  143.260709] Added buffer head 7 to (ptrval) [  143.260715] Added buffer
> > head 8 to (ptrval) [  143.260721] Added buffer head 9 to (ptrval)
> > [  143.260727] Added buffer head 10 to (ptrval) [  143.260733] Added
> > buffer head 11 to (ptrval) [  143.260738] Added buffer head 12 to (ptrval)
> > [  143.260744] Added buffer head 13 to (ptrval) [  143.260750] Added
> > buffer head 14 to (ptrval) [  143.260756] Added buffer head 15 to (ptrval)
> > [  143.260771] virtio_rpmsg_bus virtio0: status: 7 [  143.260779]
> > remoteproc remoteproc0: kicking vq index: 0 [  143.260788] remoteproc
> > remoteproc0: sending message : vqid = 0 [  143.260802] imx_mu
> > 30aa0000.mailbox: Send data on wrong channel type: 1 [  143.260810]
> > virtio_rpmsg_bus virtio0: rpmsg host is online [  143.261680]
> > imx7d-cm4#vdev0buffer: registered virtio0 (type 7) [  143.261694]
> > remoteproc remoteproc0: remote processor imx-rproc is now up
> > [  143.354880] remoteproc remoteproc0: vq index 0 is interrupted
> > [  143.354895] virtqueue callback for (ptrval) ((ptrval)) [  143.354912]
> > virtio_rpmsg_bus virtio0: From: 0x0, To: 0x35, Len: 40, Flags: 0, Reserved: 0
> > [  143.354924] rpmsg_virtio RX: 00 00 00 00 35 00 00 00 00 00 00 00 28 00
> > 00 00  ....5.......(...
> > [  143.354932] rpmsg_virtio RX: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00
> > 00 00  rpmsg-tty-raw...
> > [  143.354939] rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00  ................
> > [  143.354945] rpmsg_virtio RX: 00 00 00 00 00 00 00
> > 00                          ........
> > [  143.354956] NS announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77
> > 00 00 00  rpmsg-tty-raw...
> > [  143.354963] NS announcement: 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00  ................
> > [  143.354969] NS announcement: 00 00 00 00 00 00 00
> > 00                          ........
> > [  143.354980] virtio_rpmsg_bus virtio0: creating channel rpmsg-tty-raw
> > addr 0x0 [  143.356584] rpmsg_tty virtio0.rpmsg-tty-raw.-1.0: new channel:
> > 0x400 -> 0x0 : ttyRPMSG0 [  143.356651] Added buffer head 0 to (ptrval)
> > [  143.356658] No more buffers in queue [  143.356667] virtio_rpmsg_bus
> > virtio0: Received 1 messages [  143.404302] remoteproc remoteproc0: vq
> > index 0 is interrupted [  143.404319] virtqueue callback for (ptrval) ((ptrval))
> > [  143.404337] virtio_rpmsg_bus virtio0: From: 0x1, To: 0x35, Len: 40, Flags:
> > 0, Reserved: 0 [  143.404350] rpmsg_virtio RX: 01 00 00 00 35 00 00 00 00
> > 00 00 00 28 00 00 00  ....5.......(...
> > [  143.404391] rpmsg_virtio RX: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00
> > 00 00  rpmsg-tty-raw...
> > [  143.404399] rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00  ................
> > [  143.404405] rpmsg_virtio RX: 01 00 00 00 00 00 00
> > 00                          ........
> > [  143.404417] NS announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77
> > 00 00 00  rpmsg-tty-raw...
> > [  143.404424] NS announcement: 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00  ................
> > [  143.404430] NS announcement: 01 00 00 00 00 00 00
> > 00                          ........
> > [  143.404441] virtio_rpmsg_bus virtio0: creating channel rpmsg-tty-raw
> > addr 0x1 [  143.411114] rpmsg_tty virtio0.rpmsg-tty-raw.-1.1: new channel:
> > 0x401 -> 0x1 : ttyRPMSG1
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support
  2020-04-14 17:20     ` Mathieu Poirier
@ 2020-04-17  8:37       ` Nikita Shubin
  2020-04-17 16:02         ` Mathieu Poirier
  0 siblings, 1 reply; 29+ messages in thread
From: Nikita Shubin @ 2020-04-17  8:37 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-kernel, linux-arm-kernel

On Tue, 14 Apr 2020 11:20:05 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> On Mon, Apr 06, 2020 at 02:33:09PM +0300, nikita.shubin@maquefel.me
> wrote:
> > Add support for mailboxes to imx_rproc
> > 
> > Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> > ---
> >  drivers/remoteproc/Kconfig     |   2 +
> >  drivers/remoteproc/imx_rproc.c | 142
> > ++++++++++++++++++++++++++++++++- 2 files changed, 143
> > insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index 94afdde4bc9f..02d23a54c9cf 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -17,6 +17,8 @@ if REMOTEPROC
> >  config IMX_REMOTEPROC
> >  	tristate "IMX6/7 remoteproc support"
> >  	depends on ARCH_MXC
> > +	select MAILBOX
> > +	select IMX_MBOX
> >  	help
> >  	  Say y here to support iMX's remote processors (Cortex M4
> >  	  on iMX7D) via the remote processor framework.
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index bebc58d0f711..d2bede4ccb70
> > 100644 --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -14,6 +14,9 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> >  #include <linux/remoteproc.h>
> > +#include <linux/mailbox_client.h>
> > +
> > +#include "remoteproc_internal.h"
> >  
> >  #define IMX7D_SRC_SCR			0x0C
> >  #define IMX7D_ENABLE_M4			BIT(3)
> > @@ -47,6 +50,12 @@
> >  
> >  #define IMX_BOOT_PC			0x4
> >  
> > +#define IMX_MBOX_NB_VQ			2
> > +#define IMX_MBOX_NB_MBX		2
> 
> Please align this.
> 
> > +
> > +#define IMX_MBX_VQ0		"vq0"
> > +#define IMX_MBX_VQ1		"vq1"
> > +
> >  /**
> >   * struct imx_rproc_mem - slim internal memory structure
> >   * @cpu_addr: MPU virtual address of the memory region
> > @@ -80,6 +89,14 @@ struct imx_rproc_dcfg {
> >  	size_t				att_size;
> >  };
> >  
> > +struct imx_mbox {
> > +	const unsigned char name[10];
> > +	struct mbox_chan *chan;
> > +	struct mbox_client client;
> > +	struct work_struct vq_work;
> > +	int vq_id;
> > +};
> > +
> >  struct imx_rproc {
> >  	struct device			*dev;
> >  	struct regmap			*regmap;
> > @@ -88,6 +105,8 @@ struct imx_rproc {
> >  	struct imx_rproc_mem
> > mem[IMX7D_RPROC_MEM_MAX]; struct clk			*clk;
> >  	void __iomem			*bootreg;
> > +	struct imx_mbox mb[IMX_MBOX_NB_MBX];
> > +	struct workqueue_struct *workqueue;
> >  };
> >  
> >  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> > @@ -251,10 +270,118 @@ static void *imx_rproc_da_to_va(struct rproc
> > *rproc, u64 da, int len) return va;
> >  }
> >  
> > +static void imx_rproc_mb_vq_work(struct work_struct *work)
> > +{
> > +	struct imx_mbox *mb = container_of(work, struct imx_mbox,
> > vq_work);
> > +	struct rproc *rproc = dev_get_drvdata(mb->client.dev);
> > +
> > +	if (rproc_vq_interrupt(rproc, mb->vq_id) == IRQ_NONE)
> > +		dev_dbg(&rproc->dev, "no message found in vq%d\n",
> > mb->vq_id); +}
> > +
> > +static void imx_rproc_mb_callback(struct mbox_client *cl, void
> > *data) +{
> > +	struct rproc *rproc = dev_get_drvdata(cl->dev);
> > +	struct imx_mbox *mb = container_of(cl, struct imx_mbox,
> > client);
> > +	struct imx_rproc *ddata = rproc->priv;
> > +
> > +	queue_work(ddata->workqueue, &mb->vq_work);
> > +}
> > +
> > +static const struct imx_mbox imx_rproc_mbox[IMX_MBOX_NB_MBX] = {
> > +	{
> > +		.name = IMX_MBX_VQ0,
> > +		.vq_id = 0,
> > +		.client = {
> > +			.rx_callback = imx_rproc_mb_callback,
> > +			.tx_block = false,
> > +		},
> > +	},
> > +	{
> > +		.name = IMX_MBX_VQ1,
> > +		.vq_id = 1,
> > +		.client = {
> > +			.rx_callback = imx_rproc_mb_callback,
> > +			.tx_block = false,
> > +		},
> > +	},
> > +};
> > +
> > +static void imx_rproc_request_mbox(struct rproc *rproc)
> > +{
> > +	struct imx_rproc *ddata = rproc->priv;
> > +	struct device *dev = &rproc->dev;
> > +	unsigned int i;
> > +	const unsigned char *name;
> > +	struct mbox_client *cl;
> > +
> > +	/* Initialise mailbox structure table */
> > +	memcpy(ddata->mb, imx_rproc_mbox, sizeof(imx_rproc_mbox));
> > +
> > +	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> > +		name = ddata->mb[i].name;
> > +
> > +		cl = &ddata->mb[i].client;
> > +		cl->dev = dev->parent;
> > +
> > +		ddata->mb[i].chan =
> > mbox_request_channel_byname(cl, name); +
> > +		dev_dbg(dev, "%s: name=%s, idx=%u\n",
> > +			__func__, name, ddata->mb[i].vq_id);
> > +
> > +		if (IS_ERR(ddata->mb[i].chan)) {
> > +			dev_warn(dev, "cannot get %s mbox\n",
> > name);
> > +			ddata->mb[i].chan = NULL;
> 
> If the mailbox isn't ready this driver will fail without a chance of
> recovery. Since most of the code in this patch is a carbon copy of
> the implementation found in stm32_proc.c, I suggest you do the same
> as they did in stm32_rproc_request_mbox() and privision for cases
> where requesting a channel returns -EPROBE_DEFER.
> 

Noted, will be fixed.

> > +		}
> > +
> > +		if (ddata->mb[i].vq_id >= 0)
> > +			INIT_WORK(&ddata->mb[i].vq_work,
> > imx_rproc_mb_vq_work);
> > +	}
> > +}
> > +
> > +static void imx_rproc_free_mbox(struct rproc *rproc)
> > +{
> > +	struct imx_rproc *ddata = rproc->priv;
> > +	unsigned int i;
> > +
> > +	dev_dbg(&rproc->dev, "%s: %d boxes\n",
> > +		__func__, ARRAY_SIZE(ddata->mb));
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ddata->mb); i++) {
> > +		if (ddata->mb[i].chan)
> > +			mbox_free_channel(ddata->mb[i].chan);
> > +		ddata->mb[i].chan = NULL;
> > +	}
> > +}
> > +
> > +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> > +{
> > +	struct imx_rproc *ddata = rproc->priv;
> > +	unsigned int i;
> > +	int err;
> > +
> > +	if (WARN_ON(vqid >= IMX_MBOX_NB_VQ))
> > +		return;
> > +
> > +	for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> > +		if (vqid != ddata->mb[i].vq_id)
> > +			continue;
> > +		if (!ddata->mb[i].chan)
> > +			return;
> > +		dev_dbg(&rproc->dev, "sending message : vqid =
> > %d\n", vqid);
> > +		err = mbox_send_message(ddata->mb[i].chan, &vqid);
> > +		if (err < 0)
> > +			dev_err(&rproc->dev, "%s: failed (%s,
> > err:%d)\n",
> > +					__func__,
> > ddata->mb[i].name, err);
> > +			return;
> > +	}
> > +}
> > +
> >  static const struct rproc_ops imx_rproc_ops = {
> >  	.start		= imx_rproc_start,
> >  	.stop		= imx_rproc_stop,
> >  	.da_to_va	= imx_rproc_da_to_va,
> > +	.kick		= imx_rproc_kick,
> >  	.get_boot_addr	= rproc_elf_get_boot_addr,
> >  };
> >  
> > @@ -384,14 +511,26 @@ static int imx_rproc_probe(struct
> > platform_device *pdev) goto err_put_rproc;
> >  	}
> >  
> > +	priv->workqueue = create_workqueue(dev_name(dev));
> > +	if (!priv->workqueue) {
> > +		dev_err(dev, "cannot create workqueue\n");
> > +		ret = -ENOMEM;
> > +		goto err_put_clk;
> > +	}
> > +
> > +	imx_rproc_request_mbox(rproc);
> > +
> >  	ret = rproc_add(rproc);
> >  	if (ret) {
> >  		dev_err(dev, "rproc_add failed\n");
> > -		goto err_put_clk;
> > +		goto err_free_mb;
> >  	}
> >  
> >  	return 0;
> >  
> > +err_free_mb:
> > +	imx_rproc_free_mbox(rproc);
> > +	destroy_workqueue(priv->workqueue);
> >  err_put_clk:
> >  	clk_disable_unprepare(priv->clk);
> >  err_put_rproc:
> > @@ -407,6 +546,7 @@ static int imx_rproc_remove(struct
> > platform_device *pdev) 
> >  	clk_disable_unprepare(priv->clk);
> >  	rproc_del(rproc);
> > +	imx_rproc_free_mbox(rproc);
> 
> I have no issues with people reusing code already found in the kernel
> - in fact I encourage it because it makes reviewing patches much
> easier.  On the flip side you have to give credit where it is due.
> Here adding a line in the changelog that mentions where you took your
> inspiration from will be much appreciated.

Please don't blame on things i never did citing my own self from 0/0:

| Regarding mailboxes and memory regions :

| This code is heavily derived from stm32-rproc (i.e. copy pasted) and
| this fact needs to reflected in commits, please tell me how to
| emphasize this fact.

I am eager to give credits.


> 
> Thanks,
> Mathieu
> 
> >  	rproc_free(rproc);
> >  
> >  	return 0;
> > -- 
> > 2.25.1
> > 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support
  2020-04-15  2:42   ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support Peng Fan
  2020-04-15 16:26     ` Mathieu Poirier
@ 2020-04-17  8:57     ` Nikita Shubin
  1 sibling, 0 replies; 29+ messages in thread
From: Nikita Shubin @ 2020-04-17  8:57 UTC (permalink / raw)
  To: Peng Fan
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, dl-linux-imx,
	linux-remoteproc, linux-arm-kernel, linux-kernel

On Wed, 15 Apr 2020 02:42:32 +0000
Peng Fan <peng.fan@nxp.com> wrote:

> > Subject: [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support
> 
> Have you ever see https://patchwork.kernel.org/cover/11390477/?
> 

I don't see anything that allows booting imx7-m4 from A7 In case elf
file interrupt vector is not supposed to be at OCRAM_S, am i missing
something?

I not interested in booting A7 from M4, i interested in wise versa
scenario.

> I have been waiting for Mathieu's rproc sync state patch, then
> rebase.
> 
> Thanks,
> Peng.
> 
> > 
> > This patch set introduces virtio support for imx7d-m4 communication:
> > 
> > - support booting loaded vim imx-rproc firmware
> > - implement .kick method support using mailbox in imx-processor
> > - parse vdev0vring0, vdev0vring1, vdev0buffer memory regions
> > required for virtio_rpmsg_bus initialization
> > 
> > Regarding imx7d-m4 boot proccess
> > 
> > Citing ARM documentation:
> > 
> > At Reset, Cortex-M3 and Cortex-M4 processors always boot from a
> > vector table at address zero.
> > 
> > "With uninitialized memory at address zero (for example,
> > unprogrammed Flash or uninitialized RAM), the processor will read a
> > spurious initial Main Stack Pointer value from address zero and a
> > spurious code entry point (Reset vector) from address 0x4, possibly
> > containing an illegal instruction set state specifier (ESPR.T bit)
> > in bit[0]."
> > 
> > So to successfully boot m4 coproc we need to write Stack Pointer and
> > Program counter, i see no obvious to get Stack Pointer value, so
> > two ways exist ethier form a special elf section:
> > 
> > "
> > .loader :
> >   {
> >     LONG(__StackTop);
> >     LONG(Reset_Handler + 1);
> >   } > m_start
> > "
> > 
> > and put it at 0x0 address:
> > 
> > "
> > m_start               (RX)  : ORIGIN = 0x00000000, LENGTH =
> > 0x00008000
> > "
> > 
> > Or (the way i've chosen) only put Entry Point at 0x04 and set stack
> > as first instruction:
> > 
> > "
> > Reset_Handler:
> > 	ldr   sp, =__stack      /* set stack pointer */
> > "
> > 
> > Regarding mailboxes and memory regions :
> > 
> > This code is heavily derived from stm32-rproc (i.e. copy pasted)
> > and this fact needs to reflected in commits, please tell me how to
> > emphasize this fact.
> > 
> > Attaching succesful trace booting m4 (with Add rpmsg tty driver
> > applied) :
> > 
> > [  143.240616] remoteproc remoteproc0: powering up imx-rproc
> > [  143.251768] remoteproc remoteproc0: Booting fw image huginn.elf,
> > size 466876 [  143.251786] imx-rproc imx7d-cm4: iommu not present
> > [  143.251825] remoteproc remoteproc0: rsc: type 3 [  143.251837]
> > remoteproc remoteproc0: vdev rsc: id 7, dfeatures 0x1, cfg len 0, 2
> > vrings [  143.251924] remoteproc remoteproc0: vdev rsc: vring0: da
> > 0xffffffff, qsz 16, align 16 [  143.251935] remoteproc remoteproc0:
> > vdev rsc: vring1: da 0xffffffff, qsz 16, align 16 [  143.251955]
> > imx-rproc imx7d-cm4: map memory: 0x00900000+20000 [  143.251987]
> > imx-rproc imx7d-cm4: map memory: 0x00920000+2000 [  143.252003]
> > imx-rproc imx7d-cm4: map memory: 0x00922000+2000 [  143.252020]
> > remoteproc remoteproc0: phdr: type 1 da 0x20200000 memsz 0x240
> > filesz 0x240 [  143.252032] remoteproc remoteproc0: da = 0x20200000
> > len = 0x240 va = 0x(ptrval) [  143.252043] remoteproc remoteproc0:
> > phdr: type 1 da 0x20200240 memsz 0x5b38 filesz 0x5b38 [
> > 143.252053] remoteproc remoteproc0: da = 0x20200240 len = 0x5b38 va
> > = 0x(ptrval) [  143.252105] remoteproc remoteproc0: phdr: type 1 da
> > 0x20205d78 memsz 0x4b58 filesz 0x758 [  143.252115] remoteproc
> > remoteproc0: da = 0x20205d78 len = 0x4b58 va = 0x(ptrval) [
> > 143.252159] remoteproc remoteproc0: da = 0x200006cc len = 0x8c va =
> > 0x(ptrval) [  143.252176] remoteproc remoteproc0: Started from
> > 0x202002f5 [  143.252211]  imx7d-cm4#vdev0buffer: assigned reserved
> > memory node vdev0buffer@00924000 [  143.252232] virtio virtio0:
> > reset ! [  143.252241] virtio virtio0: status: 1 [  143.260567]
> > virtio_rpmsg_bus virtio0: status: 3 [  143.260598] remoteproc
> > remoteproc0: vring0: va c083c000 qsz 16 notifyid 0 [  143.260614]
> > remoteproc remoteproc0: vring1: va c0872000 qsz 16 notifyid 1 [
> > 143.260651] virtio_rpmsg_bus virtio0: buffers: va c0894000, dma
> > 0x00924000 [  143.260666] Added buffer head 0 to (ptrval) [
> > 143.260674] Added buffer head 1 to (ptrval) [  143.260680] Added
> > buffer head 2 to (ptrval) [  143.260686] Added buffer head 3 to
> > (ptrval) [  143.260692] Added buffer head 4 to (ptrval) [
> > 143.260697] Added buffer head 5 to (ptrval) [  143.260703] Added
> > buffer head 6 to (ptrval) [  143.260709] Added buffer head 7 to
> > (ptrval) [  143.260715] Added buffer head 8 to (ptrval) [
> > 143.260721] Added buffer head 9 to (ptrval) [  143.260727] Added
> > buffer head 10 to (ptrval) [  143.260733] Added buffer head 11 to
> > (ptrval) [  143.260738] Added buffer head 12 to (ptrval) [
> > 143.260744] Added buffer head 13 to (ptrval) [  143.260750] Added
> > buffer head 14 to (ptrval) [  143.260756] Added buffer head 15 to
> > (ptrval) [  143.260771] virtio_rpmsg_bus virtio0: status: 7 [
> > 143.260779] remoteproc remoteproc0: kicking vq index: 0 [
> > 143.260788] remoteproc remoteproc0: sending message : vqid = 0 [
> > 143.260802] imx_mu 30aa0000.mailbox: Send data on wrong channel
> > type: 1 [  143.260810] virtio_rpmsg_bus virtio0: rpmsg host is
> > online [  143.261680] imx7d-cm4#vdev0buffer: registered virtio0
> > (type 7) [  143.261694] remoteproc remoteproc0: remote processor
> > imx-rproc is now up [  143.354880] remoteproc remoteproc0: vq index
> > 0 is interrupted [  143.354895] virtqueue callback for (ptrval)
> > ((ptrval)) [  143.354912] virtio_rpmsg_bus virtio0: From: 0x0, To:
> > 0x35, Len: 40, Flags: 0, Reserved: 0 [  143.354924] rpmsg_virtio
> > RX: 00 00 00 00 35 00 00 00 00 00 00 00 28 00 00 00
> > ....5.......(... [  143.354932] rpmsg_virtio RX: 72 70 6d 73 67 2d
> > 74 74 79 2d 72 61 77 00 00 00  rpmsg-tty-raw... [  143.354939]
> > rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ................ [  143.354945] rpmsg_virtio RX: 00 00 00 00 00 00
> > 00 00                          ........ [  143.354956] NS
> > announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61 77 00 00 00
> > rpmsg-tty-raw... [  143.354963] NS announcement: 00 00 00 00 00 00
> > 00 00 00 00 00 00 00 00 00 00  ................ [  143.354969] NS
> > announcement: 00 00 00 00 00 00 00 00
> > ........ [  143.354980] virtio_rpmsg_bus virtio0: creating channel
> > rpmsg-tty-raw addr 0x0 [  143.356584] rpmsg_tty
> > virtio0.rpmsg-tty-raw.-1.0: new channel: 0x400 -> 0x0 : ttyRPMSG0 [
> >  143.356651] Added buffer head 0 to (ptrval) [  143.356658] No more
> > buffers in queue [  143.356667] virtio_rpmsg_bus virtio0: Received
> > 1 messages [  143.404302] remoteproc remoteproc0: vq index 0 is
> > interrupted [  143.404319] virtqueue callback for (ptrval)
> > ((ptrval)) [  143.404337] virtio_rpmsg_bus virtio0: From: 0x1, To:
> > 0x35, Len: 40, Flags: 0, Reserved: 0 [  143.404350] rpmsg_virtio
> > RX: 01 00 00 00 35 00 00 00 00 00 00 00 28 00 00 00
> > ....5.......(... [  143.404391] rpmsg_virtio RX: 72 70 6d 73 67 2d
> > 74 74 79 2d 72 61 77 00 00 00  rpmsg-tty-raw... [  143.404399]
> > rpmsg_virtio RX: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ................ [  143.404405] rpmsg_virtio RX: 01 00 00 00 00 00
> > 00 00                          ........
> > [  143.404417] NS announcement: 72 70 6d 73 67 2d 74 74 79 2d 72 61
> > 77 00 00 00  rpmsg-tty-raw...
> > [  143.404424] NS announcement: 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00 00  ................
> > [  143.404430] NS announcement: 01 00 00 00 00 00 00
> > 00                          ........
> > [  143.404441] virtio_rpmsg_bus virtio0: creating channel
> > rpmsg-tty-raw addr 0x1 [  143.411114] rpmsg_tty
> > virtio0.rpmsg-tty-raw.-1.1: new channel: 0x401 -> 0x1 : ttyRPMSG1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-14 16:45     ` Mathieu Poirier
@ 2020-04-17 12:11       ` Nikita Shubin
  2020-04-17 15:37         ` Mathieu Poirier
       [not found]       ` <45761587100993@mail.yandex.ru>
  1 sibling, 1 reply; 29+ messages in thread
From: Nikita Shubin @ 2020-04-17 12:11 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

On Tue, 14 Apr 2020 10:45:19 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> Hi Nikita,
> 
> On Mon, Apr 06, 2020 at 02:33:08PM +0300, nikita.shubin@maquefel.me
> wrote:
> > In case elf file interrupt vector is not supposed to be at OCRAM_S,
> > it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
> > firmware.
> > 
> > Otherwise firmware located anywhere besides OCRAM_S won't boot.
> > 
> > The firmware must set stack poiner as first instruction:
> > 
> > Reset_Handler:
> >     ldr   sp, = __stack      /* set stack pointer */
> > 
> > Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> 
> The address in the SoB has to match what is found in the "From:"
> field of the email header.  Checkpatch is complaining about that,
> something I would have expected to be fixed before sending this set
> out.
> 
> > ---
> >  drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 3e72b6f38d4b..bebc58d0f711
> > 100644 --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -45,6 +45,8 @@
> >  
> >  #define IMX7D_RPROC_MEM_MAX		8
> >  
> > +#define IMX_BOOT_PC			0x4
> > +
> >  /**
> >   * struct imx_rproc_mem - slim internal memory structure
> >   * @cpu_addr: MPU virtual address of the memory region
> > @@ -85,6 +87,7 @@ struct imx_rproc {
> >  	const struct imx_rproc_dcfg	*dcfg;
> >  	struct imx_rproc_mem
> > mem[IMX7D_RPROC_MEM_MAX]; struct clk			*clk;
> > +	void __iomem			*bootreg;
> >  };
> >  
> >  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> > @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc
> > *rproc) struct device *dev = priv->dev;
> >  	int ret;
> >  
> > +	/* write entry point to program counter */
> > +	writel(rproc->bootaddr, priv->bootreg);
> 
> What happens on all the other IMX systems where this fix is not
> needed?  Will they continue to work properly?   

Mathieu you are totally correct imx6/imx7 use different addresses they
boot.

For imx7:
| On i.MX 7Dual/7Solo, the boot vector for the Cortex-M4 core is located
| at the start of the OCRAM_S (On Chip RAM - Secure) whose address is
| 0x0018_0000 from Cortex-A7.

For imx6:
| The Boot vector for the Cortex-M4 core is located at the start of the
| TCM_L whose address is 0x007F_8000 from the Cortex-A9. This is a
| different location than on the i.MX 7Dual/7Solo

But on imx7 0x0 is translated to 0x0018_0000 by imx_rproc_da_to_va, and
on imx7 0x0 is translated to 0x007F_8000, using imx_rproc_att_imx7d and 
imx_rproc_att_imx6sx respectively.

I have no information about IMX8 (i have found none available
publicity), but should be the same as Cortex-M boots from 0x0.

> 
> > +
> >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> >  				 dcfg->src_mask, dcfg->src_start);
> >  	if (ret)
> >  		dev_err(dev, "Failed to enable M4!\n");
> >  
> > +	dev_info(&rproc->dev, "Started from 0x%x\n",
> > rproc->bootaddr); +
> >  	return ret;
> >  }
> >  
> > @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> >  	if (ret)
> >  		dev_err(dev, "Failed to stop M4!\n");
> >  
> > +	/* clear entry points */
> > +	writel(0, priv->bootreg);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc
> > *rproc, u64 da, int len) static const struct rproc_ops
> > imx_rproc_ops = { .start		= imx_rproc_start,
> >  	.stop		= imx_rproc_stop,
> > -	.da_to_va       = imx_rproc_da_to_va,
> > +	.da_to_va	= imx_rproc_da_to_va,
> > +	.get_boot_addr	= rproc_elf_get_boot_addr,
> 
> How is this useful?  Sure it will set rproc->bootaddr in
> rproc_fw_boot() but what good does that do when it is invariably set
> again in imx_rproc_start() ? 
> 
> >  };
> >  
> >  static int imx_rproc_addr_init(struct imx_rproc *priv,
> > @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct
> > platform_device *pdev) goto err_put_rproc;
> >  	}
> >  
> > +	priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC,
> > sizeof(u32)); +
> >  	/*
> >  	 * clk for M4 block including memory. Should be
> >  	 * enabled before .start for FW transfer.
> > -- 
> > 2.25.1
> > 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-17 12:11       ` Nikita Shubin
@ 2020-04-17 15:37         ` Mathieu Poirier
  2020-04-17 15:46           ` Nikita Shubin
  0 siblings, 1 reply; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-17 15:37 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel,
	Linux Kernel Mailing List

On Fri, 17 Apr 2020 at 06:12, Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> On Tue, 14 Apr 2020 10:45:19 -0600
> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> > Hi Nikita,
> >
> > On Mon, Apr 06, 2020 at 02:33:08PM +0300, nikita.shubin@maquefel.me
> > wrote:
> > > In case elf file interrupt vector is not supposed to be at OCRAM_S,
> > > it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
> > > firmware.
> > >
> > > Otherwise firmware located anywhere besides OCRAM_S won't boot.
> > >
> > > The firmware must set stack poiner as first instruction:
> > >
> > > Reset_Handler:
> > >     ldr   sp, = __stack      /* set stack pointer */
> > >
> > > Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> >
> > The address in the SoB has to match what is found in the "From:"
> > field of the email header.  Checkpatch is complaining about that,
> > something I would have expected to be fixed before sending this set
> > out.
> >
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index 3e72b6f38d4b..bebc58d0f711
> > > 100644 --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -45,6 +45,8 @@
> > >
> > >  #define IMX7D_RPROC_MEM_MAX                8
> > >
> > > +#define IMX_BOOT_PC                        0x4
> > > +
> > >  /**
> > >   * struct imx_rproc_mem - slim internal memory structure
> > >   * @cpu_addr: MPU virtual address of the memory region
> > > @@ -85,6 +87,7 @@ struct imx_rproc {
> > >     const struct imx_rproc_dcfg     *dcfg;
> > >     struct imx_rproc_mem
> > > mem[IMX7D_RPROC_MEM_MAX]; struct clk                        *clk;
> > > +   void __iomem                    *bootreg;
> > >  };
> > >
> > >  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> > > @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc
> > > *rproc) struct device *dev = priv->dev;
> > >     int ret;
> > >
> > > +   /* write entry point to program counter */
> > > +   writel(rproc->bootaddr, priv->bootreg);
> >
> > What happens on all the other IMX systems where this fix is not
> > needed?  Will they continue to work properly?
>
> Mathieu you are totally correct imx6/imx7 use different addresses they
> boot.
>
> For imx7:
> | On i.MX 7Dual/7Solo, the boot vector for the Cortex-M4 core is located
> | at the start of the OCRAM_S (On Chip RAM - Secure) whose address is
> | 0x0018_0000 from Cortex-A7.
>
> For imx6:
> | The Boot vector for the Cortex-M4 core is located at the start of the
> | TCM_L whose address is 0x007F_8000 from the Cortex-A9. This is a
> | different location than on the i.MX 7Dual/7Solo
>
> But on imx7 0x0 is translated to 0x0018_0000 by imx_rproc_da_to_va, and
> on imx7 0x0 is translated to 0x007F_8000, using imx_rproc_att_imx7d and
> imx_rproc_att_imx6sx respectively.

My point here is that before your patch, this driver was running on
IMX platforms.  How does your work impact existing platforms that are
booting properly?

>
> I have no information about IMX8 (i have found none available
> publicity), but should be the same as Cortex-M boots from 0x0.
>
> >
> > > +
> > >     ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > >                              dcfg->src_mask, dcfg->src_start);
> > >     if (ret)
> > >             dev_err(dev, "Failed to enable M4!\n");
> > >
> > > +   dev_info(&rproc->dev, "Started from 0x%x\n",
> > > rproc->bootaddr); +
> > >     return ret;
> > >  }
> > >
> > > @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> > >     if (ret)
> > >             dev_err(dev, "Failed to stop M4!\n");
> > >
> > > +   /* clear entry points */
> > > +   writel(0, priv->bootreg);
> > > +
> > >     return ret;
> > >  }
> > >
> > > @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc
> > > *rproc, u64 da, int len) static const struct rproc_ops
> > > imx_rproc_ops = { .start            = imx_rproc_start,
> > >     .stop           = imx_rproc_stop,
> > > -   .da_to_va       = imx_rproc_da_to_va,
> > > +   .da_to_va       = imx_rproc_da_to_va,
> > > +   .get_boot_addr  = rproc_elf_get_boot_addr,
> >
> > How is this useful?  Sure it will set rproc->bootaddr in
> > rproc_fw_boot() but what good does that do when it is invariably set
> > again in imx_rproc_start() ?
> >
> > >  };
> > >
> > >  static int imx_rproc_addr_init(struct imx_rproc *priv,
> > > @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct
> > > platform_device *pdev) goto err_put_rproc;
> > >     }
> > >
> > > +   priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC,
> > > sizeof(u32)); +
> > >     /*
> > >      * clk for M4 block including memory. Should be
> > >      * enabled before .start for FW transfer.
> > > --
> > > 2.25.1
> > >
>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-17 15:37         ` Mathieu Poirier
@ 2020-04-17 15:46           ` Nikita Shubin
  0 siblings, 0 replies; 29+ messages in thread
From: Nikita Shubin @ 2020-04-17 15:46 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel,
	Linux Kernel Mailing List

On Fri, 17 Apr 2020 09:37:42 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> On Fri, 17 Apr 2020 at 06:12, Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > On Tue, 14 Apr 2020 10:45:19 -0600
> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >
> > > Hi Nikita,
> > >
> > > On Mon, Apr 06, 2020 at 02:33:08PM +0300,
> > > nikita.shubin@maquefel.me wrote:
> > > > In case elf file interrupt vector is not supposed to be at
> > > > OCRAM_S, it is needed to write elf entry point to OCRAM_S +
> > > > 0x4, to boot M4 firmware.
> > > >
> > > > Otherwise firmware located anywhere besides OCRAM_S won't boot.
> > > >
> > > > The firmware must set stack poiner as first instruction:
> > > >
> > > > Reset_Handler:
> > > >     ldr   sp, = __stack      /* set stack pointer */
> > > >
> > > > Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> > >
> > > The address in the SoB has to match what is found in the "From:"
> > > field of the email header.  Checkpatch is complaining about that,
> > > something I would have expected to be fixed before sending this
> > > set out.
> > >
> > > > ---
> > > >  drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > > b/drivers/remoteproc/imx_rproc.c index
> > > > 3e72b6f38d4b..bebc58d0f711 100644 ---
> > > > a/drivers/remoteproc/imx_rproc.c +++
> > > > b/drivers/remoteproc/imx_rproc.c @@ -45,6 +45,8 @@
> > > >
> > > >  #define IMX7D_RPROC_MEM_MAX                8
> > > >
> > > > +#define IMX_BOOT_PC                        0x4
> > > > +
> > > >  /**
> > > >   * struct imx_rproc_mem - slim internal memory structure
> > > >   * @cpu_addr: MPU virtual address of the memory region
> > > > @@ -85,6 +87,7 @@ struct imx_rproc {
> > > >     const struct imx_rproc_dcfg     *dcfg;
> > > >     struct imx_rproc_mem
> > > > mem[IMX7D_RPROC_MEM_MAX]; struct clk
> > > > *clk;
> > > > +   void __iomem                    *bootreg;
> > > >  };
> > > >
> > > >  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> > > > @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc
> > > > *rproc) struct device *dev = priv->dev;
> > > >     int ret;
> > > >
> > > > +   /* write entry point to program counter */
> > > > +   writel(rproc->bootaddr, priv->bootreg);
> > >
> > > What happens on all the other IMX systems where this fix is not
> > > needed?  Will they continue to work properly?
> >
> > Mathieu you are totally correct imx6/imx7 use different addresses
> > they boot.
> >
> > For imx7:
> > | On i.MX 7Dual/7Solo, the boot vector for the Cortex-M4 core is
> > located | at the start of the OCRAM_S (On Chip RAM - Secure) whose
> > address is | 0x0018_0000 from Cortex-A7.
> >
> > For imx6:
> > | The Boot vector for the Cortex-M4 core is located at the start of
> > the | TCM_L whose address is 0x007F_8000 from the Cortex-A9. This
> > is a | different location than on the i.MX 7Dual/7Solo
> >
> > But on imx7 0x0 is translated to 0x0018_0000 by imx_rproc_da_to_va,
> > and on imx7 0x0 is translated to 0x007F_8000, using
> > imx_rproc_att_imx7d and imx_rproc_att_imx6sx respectively.
> 
> My point here is that before your patch, this driver was running on
> IMX platforms.  How does your work impact existing platforms that are
> booting properly?

Well it wasn't i am pretty sure it wasn't used at all or questions
about boot process should have to be raised earlier.

If we look into the first patch introduced by Oleksij Rempel
https://lore.kernel.org/patchwork/cover/799614/
we can that the program he used is located at 0x0018_0000 so he didn't
have any problems with Entry Point and stack pointer being at
0x0018_0000 and 0x0018_0004 respectively.

But as i am trying to emphasize, IF elf is not supposed to be located
starting at 0x0018_0000 it won't boot at all.

Citing Oleksij:

| no, currently my priority is to provide basic functionality with easy 
| understandable target code and dependencies.

Moreover it seems not tested on IMX6 by anyone. 

I can limit functionality only to IMX7, until i lay hands on IMX6
with m4 co-proc - is this is what desired ?

> 
> >
> > I have no information about IMX8 (i have found none available
> > publicity), but should be the same as Cortex-M boots from 0x0.
> >
> > >
> > > > +
> > > >     ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > > >                              dcfg->src_mask, dcfg->src_start);
> > > >     if (ret)
> > > >             dev_err(dev, "Failed to enable M4!\n");
> > > >
> > > > +   dev_info(&rproc->dev, "Started from 0x%x\n",
> > > > rproc->bootaddr); +
> > > >     return ret;
> > > >  }
> > > >
> > > > @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc
> > > > *rproc) if (ret)
> > > >             dev_err(dev, "Failed to stop M4!\n");
> > > >
> > > > +   /* clear entry points */
> > > > +   writel(0, priv->bootreg);
> > > > +
> > > >     return ret;
> > > >  }
> > > >
> > > > @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc
> > > > *rproc, u64 da, int len) static const struct rproc_ops
> > > > imx_rproc_ops = { .start            = imx_rproc_start,
> > > >     .stop           = imx_rproc_stop,
> > > > -   .da_to_va       = imx_rproc_da_to_va,
> > > > +   .da_to_va       = imx_rproc_da_to_va,
> > > > +   .get_boot_addr  = rproc_elf_get_boot_addr,
> > >
> > > How is this useful?  Sure it will set rproc->bootaddr in
> > > rproc_fw_boot() but what good does that do when it is invariably
> > > set again in imx_rproc_start() ?
> > >
> > > >  };
> > > >
> > > >  static int imx_rproc_addr_init(struct imx_rproc *priv,
> > > > @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct
> > > > platform_device *pdev) goto err_put_rproc;
> > > >     }
> > > >
> > > > +   priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC,
> > > > sizeof(u32)); +
> > > >     /*
> > > >      * clk for M4 block including memory. Should be
> > > >      * enabled before .start for FW transfer.
> > > > --
> > > > 2.25.1
> > > >
> >


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support
  2020-04-17  8:37       ` Nikita Shubin
@ 2020-04-17 16:02         ` Mathieu Poirier
  0 siblings, 0 replies; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-17 16:02 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, Linux Kernel Mailing List,
	linux-arm-kernel

On Fri, 17 Apr 2020 at 02:38, Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> On Tue, 14 Apr 2020 11:20:05 -0600
> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> > On Mon, Apr 06, 2020 at 02:33:09PM +0300, nikita.shubin@maquefel.me
> > wrote:
> > > Add support for mailboxes to imx_rproc
> > >
> > > Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> > > ---
> > >  drivers/remoteproc/Kconfig     |   2 +
> > >  drivers/remoteproc/imx_rproc.c | 142
> > > ++++++++++++++++++++++++++++++++- 2 files changed, 143
> > > insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > > index 94afdde4bc9f..02d23a54c9cf 100644
> > > --- a/drivers/remoteproc/Kconfig
> > > +++ b/drivers/remoteproc/Kconfig
> > > @@ -17,6 +17,8 @@ if REMOTEPROC
> > >  config IMX_REMOTEPROC
> > >     tristate "IMX6/7 remoteproc support"
> > >     depends on ARCH_MXC
> > > +   select MAILBOX
> > > +   select IMX_MBOX
> > >     help
> > >       Say y here to support iMX's remote processors (Cortex M4
> > >       on iMX7D) via the remote processor framework.
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index bebc58d0f711..d2bede4ccb70
> > > 100644 --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -14,6 +14,9 @@
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/remoteproc.h>
> > > +#include <linux/mailbox_client.h>
> > > +
> > > +#include "remoteproc_internal.h"
> > >
> > >  #define IMX7D_SRC_SCR                      0x0C
> > >  #define IMX7D_ENABLE_M4                    BIT(3)
> > > @@ -47,6 +50,12 @@
> > >
> > >  #define IMX_BOOT_PC                        0x4
> > >
> > > +#define IMX_MBOX_NB_VQ                     2
> > > +#define IMX_MBOX_NB_MBX            2
> >
> > Please align this.
> >
> > > +
> > > +#define IMX_MBX_VQ0                "vq0"
> > > +#define IMX_MBX_VQ1                "vq1"
> > > +
> > >  /**
> > >   * struct imx_rproc_mem - slim internal memory structure
> > >   * @cpu_addr: MPU virtual address of the memory region
> > > @@ -80,6 +89,14 @@ struct imx_rproc_dcfg {
> > >     size_t                          att_size;
> > >  };
> > >
> > > +struct imx_mbox {
> > > +   const unsigned char name[10];
> > > +   struct mbox_chan *chan;
> > > +   struct mbox_client client;
> > > +   struct work_struct vq_work;
> > > +   int vq_id;
> > > +};
> > > +
> > >  struct imx_rproc {
> > >     struct device                   *dev;
> > >     struct regmap                   *regmap;
> > > @@ -88,6 +105,8 @@ struct imx_rproc {
> > >     struct imx_rproc_mem
> > > mem[IMX7D_RPROC_MEM_MAX]; struct clk                        *clk;
> > >     void __iomem                    *bootreg;
> > > +   struct imx_mbox mb[IMX_MBOX_NB_MBX];
> > > +   struct workqueue_struct *workqueue;
> > >  };
> > >
> > >  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> > > @@ -251,10 +270,118 @@ static void *imx_rproc_da_to_va(struct rproc
> > > *rproc, u64 da, int len) return va;
> > >  }
> > >
> > > +static void imx_rproc_mb_vq_work(struct work_struct *work)
> > > +{
> > > +   struct imx_mbox *mb = container_of(work, struct imx_mbox,
> > > vq_work);
> > > +   struct rproc *rproc = dev_get_drvdata(mb->client.dev);
> > > +
> > > +   if (rproc_vq_interrupt(rproc, mb->vq_id) == IRQ_NONE)
> > > +           dev_dbg(&rproc->dev, "no message found in vq%d\n",
> > > mb->vq_id); +}
> > > +
> > > +static void imx_rproc_mb_callback(struct mbox_client *cl, void
> > > *data) +{
> > > +   struct rproc *rproc = dev_get_drvdata(cl->dev);
> > > +   struct imx_mbox *mb = container_of(cl, struct imx_mbox,
> > > client);
> > > +   struct imx_rproc *ddata = rproc->priv;
> > > +
> > > +   queue_work(ddata->workqueue, &mb->vq_work);
> > > +}
> > > +
> > > +static const struct imx_mbox imx_rproc_mbox[IMX_MBOX_NB_MBX] = {
> > > +   {
> > > +           .name = IMX_MBX_VQ0,
> > > +           .vq_id = 0,
> > > +           .client = {
> > > +                   .rx_callback = imx_rproc_mb_callback,
> > > +                   .tx_block = false,
> > > +           },
> > > +   },
> > > +   {
> > > +           .name = IMX_MBX_VQ1,
> > > +           .vq_id = 1,
> > > +           .client = {
> > > +                   .rx_callback = imx_rproc_mb_callback,
> > > +                   .tx_block = false,
> > > +           },
> > > +   },
> > > +};
> > > +
> > > +static void imx_rproc_request_mbox(struct rproc *rproc)
> > > +{
> > > +   struct imx_rproc *ddata = rproc->priv;
> > > +   struct device *dev = &rproc->dev;
> > > +   unsigned int i;
> > > +   const unsigned char *name;
> > > +   struct mbox_client *cl;
> > > +
> > > +   /* Initialise mailbox structure table */
> > > +   memcpy(ddata->mb, imx_rproc_mbox, sizeof(imx_rproc_mbox));
> > > +
> > > +   for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> > > +           name = ddata->mb[i].name;
> > > +
> > > +           cl = &ddata->mb[i].client;
> > > +           cl->dev = dev->parent;
> > > +
> > > +           ddata->mb[i].chan =
> > > mbox_request_channel_byname(cl, name); +
> > > +           dev_dbg(dev, "%s: name=%s, idx=%u\n",
> > > +                   __func__, name, ddata->mb[i].vq_id);
> > > +
> > > +           if (IS_ERR(ddata->mb[i].chan)) {
> > > +                   dev_warn(dev, "cannot get %s mbox\n",
> > > name);
> > > +                   ddata->mb[i].chan = NULL;
> >
> > If the mailbox isn't ready this driver will fail without a chance of
> > recovery. Since most of the code in this patch is a carbon copy of
> > the implementation found in stm32_proc.c, I suggest you do the same
> > as they did in stm32_rproc_request_mbox() and privision for cases
> > where requesting a channel returns -EPROBE_DEFER.
> >
>
> Noted, will be fixed.
>
> > > +           }
> > > +
> > > +           if (ddata->mb[i].vq_id >= 0)
> > > +                   INIT_WORK(&ddata->mb[i].vq_work,
> > > imx_rproc_mb_vq_work);
> > > +   }
> > > +}
> > > +
> > > +static void imx_rproc_free_mbox(struct rproc *rproc)
> > > +{
> > > +   struct imx_rproc *ddata = rproc->priv;
> > > +   unsigned int i;
> > > +
> > > +   dev_dbg(&rproc->dev, "%s: %d boxes\n",
> > > +           __func__, ARRAY_SIZE(ddata->mb));
> > > +
> > > +   for (i = 0; i < ARRAY_SIZE(ddata->mb); i++) {
> > > +           if (ddata->mb[i].chan)
> > > +                   mbox_free_channel(ddata->mb[i].chan);
> > > +           ddata->mb[i].chan = NULL;
> > > +   }
> > > +}
> > > +
> > > +static void imx_rproc_kick(struct rproc *rproc, int vqid)
> > > +{
> > > +   struct imx_rproc *ddata = rproc->priv;
> > > +   unsigned int i;
> > > +   int err;
> > > +
> > > +   if (WARN_ON(vqid >= IMX_MBOX_NB_VQ))
> > > +           return;
> > > +
> > > +   for (i = 0; i < IMX_MBOX_NB_MBX; i++) {
> > > +           if (vqid != ddata->mb[i].vq_id)
> > > +                   continue;
> > > +           if (!ddata->mb[i].chan)
> > > +                   return;
> > > +           dev_dbg(&rproc->dev, "sending message : vqid =
> > > %d\n", vqid);
> > > +           err = mbox_send_message(ddata->mb[i].chan, &vqid);
> > > +           if (err < 0)
> > > +                   dev_err(&rproc->dev, "%s: failed (%s,
> > > err:%d)\n",
> > > +                                   __func__,
> > > ddata->mb[i].name, err);
> > > +                   return;
> > > +   }
> > > +}
> > > +
> > >  static const struct rproc_ops imx_rproc_ops = {
> > >     .start          = imx_rproc_start,
> > >     .stop           = imx_rproc_stop,
> > >     .da_to_va       = imx_rproc_da_to_va,
> > > +   .kick           = imx_rproc_kick,
> > >     .get_boot_addr  = rproc_elf_get_boot_addr,
> > >  };
> > >
> > > @@ -384,14 +511,26 @@ static int imx_rproc_probe(struct
> > > platform_device *pdev) goto err_put_rproc;
> > >     }
> > >
> > > +   priv->workqueue = create_workqueue(dev_name(dev));
> > > +   if (!priv->workqueue) {
> > > +           dev_err(dev, "cannot create workqueue\n");
> > > +           ret = -ENOMEM;
> > > +           goto err_put_clk;
> > > +   }
> > > +
> > > +   imx_rproc_request_mbox(rproc);
> > > +
> > >     ret = rproc_add(rproc);
> > >     if (ret) {
> > >             dev_err(dev, "rproc_add failed\n");
> > > -           goto err_put_clk;
> > > +           goto err_free_mb;
> > >     }
> > >
> > >     return 0;
> > >
> > > +err_free_mb:
> > > +   imx_rproc_free_mbox(rproc);
> > > +   destroy_workqueue(priv->workqueue);
> > >  err_put_clk:
> > >     clk_disable_unprepare(priv->clk);
> > >  err_put_rproc:
> > > @@ -407,6 +546,7 @@ static int imx_rproc_remove(struct
> > > platform_device *pdev)
> > >     clk_disable_unprepare(priv->clk);
> > >     rproc_del(rproc);
> > > +   imx_rproc_free_mbox(rproc);
> >
> > I have no issues with people reusing code already found in the kernel
> > - in fact I encourage it because it makes reviewing patches much
> > easier.  On the flip side you have to give credit where it is due.
> > Here adding a line in the changelog that mentions where you took your
> > inspiration from will be much appreciated.
>
> Please don't blame on things i never did citing my own self from 0/0:

I am not blaming you at all.

>
> | Regarding mailboxes and memory regions :
>
> | This code is heavily derived from stm32-rproc (i.e. copy pasted) and
> | this fact needs to reflected in commits, please tell me how to
> | emphasize this fact.
>
> I am eager to give credits.

I didn't notice that in the original cover letter.  In the changelog,
between the description of the work and the signed-off-by and on a
line on its own, simply write that "the work is inspired from the
STM32 platform driver (drivers/remoteproc/stm32_rproc.c)".

>
>
> >
> > Thanks,
> > Mathieu
> >
> > >     rproc_free(rproc);
> > >
> > >     return 0;
> > > --
> > > 2.25.1
> > >
>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
       [not found]       ` <45761587100993@mail.yandex.ru>
@ 2020-04-17 17:01         ` Mathieu Poirier
  2020-04-17 17:26           ` Nikita Shubin
  0 siblings, 1 reply; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-17 17:01 UTC (permalink / raw)
  To: nikita.shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

On Thu, 16 Apr 2020 at 23:40, <nikita.shubin@maquefel.me> wrote:
>
> Hi Mathieue,
>
> Hi Nikita,
>
> On Mon, Apr 06, 2020 at 02:33:08PM +0300, nikita.shubin@maquefel.me wrote:
>
>  In case elf file interrupt vector is not supposed to be at OCRAM_S,
>  it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
>  firmware.
>
>  Otherwise firmware located anywhere besides OCRAM_S won't boot.
>
>  The firmware must set stack poiner as first instruction:
>
>  Reset_Handler:
>      ldr sp, = __stack /* set stack pointer */
>
>  Signed-off-by: Nikita Shubin <NShubin@topcon.com>
>
>
> The address in the SoB has to match what is found in the "From:" field of the
> email header. Checkpatch is complaining about that, something I would have
> expected to be fixed before sending this set out.
>
> Noted and will be fixed.
>
>  ---
>   drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
>  diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>  index 3e72b6f38d4b..bebc58d0f711 100644
>  --- a/drivers/remoteproc/imx_rproc.c
>  +++ b/drivers/remoteproc/imx_rproc.c
>  @@ -45,6 +45,8 @@
>
>   #define IMX7D_RPROC_MEM_MAX 8
>
>  +#define IMX_BOOT_PC 0x4
>  +
>   /**
>    * struct imx_rproc_mem - slim internal memory structure
>    * @cpu_addr: MPU virtual address of the memory region
>  @@ -85,6 +87,7 @@ struct imx_rproc {
>           const struct imx_rproc_dcfg *dcfg;
>           struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>           struct clk *clk;
>  + void __iomem *bootreg;
>   };
>
>   static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
>  @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc *rproc)
>           struct device *dev = priv->dev;
>           int ret;
>
>  + /* write entry point to program counter */
>  + writel(rproc->bootaddr, priv->bootreg);
>
>
> What happens on all the other IMX systems where this fix is not needed? Will
> they continue to work properly?
>
> Yes, my bad, it is also needed for IMX6 (but even so i need to study this topic more carefully),
> this should be applied exclusively for imx7d for now, and if will be needed someone
> with imx6 hardware to test on can extend this on imx6 also.
>
>
>
>
>  +
>           ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
>                                    dcfg->src_mask, dcfg->src_start);
>           if (ret)
>                   dev_err(dev, "Failed to enable M4!\n");
>
>  + dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
>  +
>           return ret;
>   }
>
>  @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
>           if (ret)
>                   dev_err(dev, "Failed to stop M4!\n");
>
>  + /* clear entry points */
>  + writel(0, priv->bootreg);
>  +
>           return ret;
>   }
>
>  @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>   static const struct rproc_ops imx_rproc_ops = {
>           .start = imx_rproc_start,
>           .stop = imx_rproc_stop,
>  - .da_to_va = imx_rproc_da_to_va,
>  + .da_to_va = imx_rproc_da_to_va,
>  + .get_boot_addr = rproc_elf_get_boot_addr,
>
>
> How is this useful? Sure it will set rproc->bootaddr in rproc_fw_boot() but
> what good does that do when it is invariably set again in imx_rproc_start() ?
>
> The priv->bootreg is the address where we are writing Entry Point and it is fixed,
> 0x04 address is translated to 0x00180004, so don't quite understand you we
> are writing rproc->bootaddr into priv->bootreg, not wiseversa.
>

What is your reason to set ops->get_boot_addr ?  How does that help
the work done in this patch?

>
>   };
>
>   static int imx_rproc_addr_init(struct imx_rproc *priv,
>  @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
>                   goto err_put_rproc;
>           }
>
>  + priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC, sizeof(u32));
>  +
>           /*
>            * clk for M4 block including memory. Should be
>            * enabled before .start for FW transfer.
>  --
>  2.25.1
>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-17 17:01         ` Mathieu Poirier
@ 2020-04-17 17:26           ` Nikita Shubin
  2020-04-17 22:24             ` Mathieu Poirier
  0 siblings, 1 reply; 29+ messages in thread
From: Nikita Shubin @ 2020-04-17 17:26 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

On Fri, 17 Apr 2020 11:01:22 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> On Thu, 16 Apr 2020 at 23:40, <nikita.shubin@maquefel.me> wrote:
> >
> > Hi Mathieue,
> >
> > Hi Nikita,
> >
> > On Mon, Apr 06, 2020 at 02:33:08PM +0300, nikita.shubin@maquefel.me
> > wrote:
> >
> >  In case elf file interrupt vector is not supposed to be at OCRAM_S,
> >  it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
> >  firmware.
> >
> >  Otherwise firmware located anywhere besides OCRAM_S won't boot.
> >
> >  The firmware must set stack poiner as first instruction:
> >
> >  Reset_Handler:
> >      ldr sp, = __stack /* set stack pointer */
> >
> >  Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> >
> >
> > The address in the SoB has to match what is found in the "From:"
> > field of the email header. Checkpatch is complaining about that,
> > something I would have expected to be fixed before sending this set
> > out.
> >
> > Noted and will be fixed.
> >
> >  ---
> >   drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
> >   1 file changed, 15 insertions(+), 1 deletion(-)
> >
> >  diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 3e72b6f38d4b..bebc58d0f711
> > 100644 --- a/drivers/remoteproc/imx_rproc.c
> >  +++ b/drivers/remoteproc/imx_rproc.c
> >  @@ -45,6 +45,8 @@
> >
> >   #define IMX7D_RPROC_MEM_MAX 8
> >
> >  +#define IMX_BOOT_PC 0x4
> >  +
> >   /**
> >    * struct imx_rproc_mem - slim internal memory structure
> >    * @cpu_addr: MPU virtual address of the memory region
> >  @@ -85,6 +87,7 @@ struct imx_rproc {
> >           const struct imx_rproc_dcfg *dcfg;
> >           struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
> >           struct clk *clk;
> >  + void __iomem *bootreg;
> >   };
> >
> >   static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> >  @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc
> > *rproc) struct device *dev = priv->dev;
> >           int ret;
> >
> >  + /* write entry point to program counter */
> >  + writel(rproc->bootaddr, priv->bootreg);
> >
> >
> > What happens on all the other IMX systems where this fix is not
> > needed? Will they continue to work properly?
> >
> > Yes, my bad, it is also needed for IMX6 (but even so i need to
> > study this topic more carefully), this should be applied
> > exclusively for imx7d for now, and if will be needed someone with
> > imx6 hardware to test on can extend this on imx6 also.
> >
> >
> >
> >
> >  +
> >           ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> >                                    dcfg->src_mask, dcfg->src_start);
> >           if (ret)
> >                   dev_err(dev, "Failed to enable M4!\n");
> >
> >  + dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
> >  +
> >           return ret;
> >   }
> >
> >  @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> >           if (ret)
> >                   dev_err(dev, "Failed to stop M4!\n");
> >
> >  + /* clear entry points */
> >  + writel(0, priv->bootreg);
> >  +
> >           return ret;
> >   }
> >
> >  @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc
> > *rproc, u64 da, int len) static const struct rproc_ops
> > imx_rproc_ops = { .start = imx_rproc_start,
> >           .stop = imx_rproc_stop,
> >  - .da_to_va = imx_rproc_da_to_va,
> >  + .da_to_va = imx_rproc_da_to_va,
> >  + .get_boot_addr = rproc_elf_get_boot_addr,
> >
> >
> > How is this useful? Sure it will set rproc->bootaddr in
> > rproc_fw_boot() but what good does that do when it is invariably
> > set again in imx_rproc_start() ?
> >
> > The priv->bootreg is the address where we are writing Entry Point
> > and it is fixed, 0x04 address is translated to 0x00180004, so don't
> > quite understand you we are writing rproc->bootaddr into
> > priv->bootreg, not wiseversa.
> >
> 
> What is your reason to set ops->get_boot_addr ?  How does that help
> the work done in this patch?

The reason is the following :

remoteproc_core.c:
| rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
| rproc->bootaddr = rproc_get_boot_addr(rproc, fw);

remoteproc_internal.h
| static inline
| u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware
*fw) | {
| 	if (rproc->ops->get_boot_addr)
| 		return rproc->ops->get_boot_addr(rproc, fw);
|
|	return 0;
| }

> 
> >
> >   };
> >
> >   static int imx_rproc_addr_init(struct imx_rproc *priv,
> >  @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct
> > platform_device *pdev) goto err_put_rproc;
> >           }
> >
> >  + priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC,
> > sizeof(u32)); +
> >           /*
> >            * clk for M4 block including memory. Should be
> >            * enabled before .start for FW transfer.
> >  --
> >  2.25.1
> >


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-17 17:26           ` Nikita Shubin
@ 2020-04-17 22:24             ` Mathieu Poirier
  2020-04-22  7:35               ` Nikita Shubin
  0 siblings, 1 reply; 29+ messages in thread
From: Mathieu Poirier @ 2020-04-17 22:24 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

On Fri, 17 Apr 2020 at 11:27, Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> On Fri, 17 Apr 2020 11:01:22 -0600
> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> > On Thu, 16 Apr 2020 at 23:40, <nikita.shubin@maquefel.me> wrote:
> > >
> > > Hi Mathieue,
> > >
> > > Hi Nikita,
> > >
> > > On Mon, Apr 06, 2020 at 02:33:08PM +0300, nikita.shubin@maquefel.me
> > > wrote:
> > >
> > >  In case elf file interrupt vector is not supposed to be at OCRAM_S,
> > >  it is needed to write elf entry point to OCRAM_S + 0x4, to boot M4
> > >  firmware.
> > >
> > >  Otherwise firmware located anywhere besides OCRAM_S won't boot.
> > >
> > >  The firmware must set stack poiner as first instruction:
> > >
> > >  Reset_Handler:
> > >      ldr sp, = __stack /* set stack pointer */
> > >
> > >  Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> > >
> > >
> > > The address in the SoB has to match what is found in the "From:"
> > > field of the email header. Checkpatch is complaining about that,
> > > something I would have expected to be fixed before sending this set
> > > out.
> > >
> > > Noted and will be fixed.
> > >
> > >  ---
> > >   drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
> > >   1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > >  diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index 3e72b6f38d4b..bebc58d0f711
> > > 100644 --- a/drivers/remoteproc/imx_rproc.c
> > >  +++ b/drivers/remoteproc/imx_rproc.c
> > >  @@ -45,6 +45,8 @@
> > >
> > >   #define IMX7D_RPROC_MEM_MAX 8
> > >
> > >  +#define IMX_BOOT_PC 0x4
> > >  +
> > >   /**
> > >    * struct imx_rproc_mem - slim internal memory structure
> > >    * @cpu_addr: MPU virtual address of the memory region
> > >  @@ -85,6 +87,7 @@ struct imx_rproc {
> > >           const struct imx_rproc_dcfg *dcfg;
> > >           struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
> > >           struct clk *clk;
> > >  + void __iomem *bootreg;
> > >   };
> > >
> > >   static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> > >  @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc
> > > *rproc) struct device *dev = priv->dev;
> > >           int ret;
> > >
> > >  + /* write entry point to program counter */
> > >  + writel(rproc->bootaddr, priv->bootreg);
> > >
> > >
> > > What happens on all the other IMX systems where this fix is not
> > > needed? Will they continue to work properly?
> > >
> > > Yes, my bad, it is also needed for IMX6 (but even so i need to
> > > study this topic more carefully), this should be applied
> > > exclusively for imx7d for now, and if will be needed someone with
> > > imx6 hardware to test on can extend this on imx6 also.
> > >
> > >
> > >
> > >
> > >  +
> > >           ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > >                                    dcfg->src_mask, dcfg->src_start);
> > >           if (ret)
> > >                   dev_err(dev, "Failed to enable M4!\n");
> > >
> > >  + dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
> > >  +
> > >           return ret;
> > >   }
> > >
> > >  @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> > >           if (ret)
> > >                   dev_err(dev, "Failed to stop M4!\n");
> > >
> > >  + /* clear entry points */
> > >  + writel(0, priv->bootreg);
> > >  +
> > >           return ret;
> > >   }
> > >
> > >  @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct rproc
> > > *rproc, u64 da, int len) static const struct rproc_ops
> > > imx_rproc_ops = { .start = imx_rproc_start,
> > >           .stop = imx_rproc_stop,
> > >  - .da_to_va = imx_rproc_da_to_va,
> > >  + .da_to_va = imx_rproc_da_to_va,
> > >  + .get_boot_addr = rproc_elf_get_boot_addr,
> > >
> > >
> > > How is this useful? Sure it will set rproc->bootaddr in
> > > rproc_fw_boot() but what good does that do when it is invariably
> > > set again in imx_rproc_start() ?
> > >
> > > The priv->bootreg is the address where we are writing Entry Point
> > > and it is fixed, 0x04 address is translated to 0x00180004, so don't
> > > quite understand you we are writing rproc->bootaddr into
> > > priv->bootreg, not wiseversa.
> > >
> >
> > What is your reason to set ops->get_boot_addr ?  How does that help
> > the work done in this patch?
>
> The reason is the following :
>
> remoteproc_core.c:
> | rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> | rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>
> remoteproc_internal.h
> | static inline
> | u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware
> *fw) | {
> |       if (rproc->ops->get_boot_addr)
> |               return rproc->ops->get_boot_addr(rproc, fw);
> |
> |       return 0;
> | }

And as I said above the value of rproc->bootaddr is set to
priv->bootreg in imx_rproc_stop().  What am I missing?  More over
imx_rproc_ops doesn't have a ->load() function and as such rproc_alloc
will set it to rproc_elf_get_boot_addr()

>
> >
> > >
> > >   };
> > >
> > >   static int imx_rproc_addr_init(struct imx_rproc *priv,
> > >  @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct
> > > platform_device *pdev) goto err_put_rproc;
> > >           }
> > >
> > >  + priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC,
> > > sizeof(u32)); +
> > >           /*
> > >            * clk for M4 block including memory. Should be
> > >            * enabled before .start for FW transfer.
> > >  --
> > >  2.25.1
> > >
>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start
  2020-04-17 22:24             ` Mathieu Poirier
@ 2020-04-22  7:35               ` Nikita Shubin
  0 siblings, 0 replies; 29+ messages in thread
From: Nikita Shubin @ 2020-04-22  7:35 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Nikita Shubin, Ohad Ben-Cohen, Bjorn Andersson, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-remoteproc, linux-arm-kernel, linux-kernel

On Fri, 17 Apr 2020 16:24:21 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> On Fri, 17 Apr 2020 at 11:27, Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > On Fri, 17 Apr 2020 11:01:22 -0600
> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >
> > > On Thu, 16 Apr 2020 at 23:40, <nikita.shubin@maquefel.me> wrote:
> > > >
> > > > Hi Mathieue,
> > > >
> > > > Hi Nikita,
> > > >
> > > > On Mon, Apr 06, 2020 at 02:33:08PM +0300,
> > > > nikita.shubin@maquefel.me wrote:
> > > >
> > > >  In case elf file interrupt vector is not supposed to be at
> > > > OCRAM_S, it is needed to write elf entry point to OCRAM_S +
> > > > 0x4, to boot M4 firmware.
> > > >
> > > >  Otherwise firmware located anywhere besides OCRAM_S won't boot.
> > > >
> > > >  The firmware must set stack poiner as first instruction:
> > > >
> > > >  Reset_Handler:
> > > >      ldr sp, = __stack /* set stack pointer */
> > > >
> > > >  Signed-off-by: Nikita Shubin <NShubin@topcon.com>
> > > >
> > > >
> > > > The address in the SoB has to match what is found in the "From:"
> > > > field of the email header. Checkpatch is complaining about that,
> > > > something I would have expected to be fixed before sending this
> > > > set out.
> > > >
> > > > Noted and will be fixed.
> > > >
> > > >  ---
> > > >   drivers/remoteproc/imx_rproc.c | 16 +++++++++++++++-
> > > >   1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > >  diff --git a/drivers/remoteproc/imx_rproc.c
> > > > b/drivers/remoteproc/imx_rproc.c index
> > > > 3e72b6f38d4b..bebc58d0f711 100644 ---
> > > > a/drivers/remoteproc/imx_rproc.c +++
> > > > b/drivers/remoteproc/imx_rproc.c @@ -45,6 +45,8 @@
> > > >
> > > >   #define IMX7D_RPROC_MEM_MAX 8
> > > >
> > > >  +#define IMX_BOOT_PC 0x4
> > > >  +
> > > >   /**
> > > >    * struct imx_rproc_mem - slim internal memory structure
> > > >    * @cpu_addr: MPU virtual address of the memory region
> > > >  @@ -85,6 +87,7 @@ struct imx_rproc {
> > > >           const struct imx_rproc_dcfg *dcfg;
> > > >           struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
> > > >           struct clk *clk;
> > > >  + void __iomem *bootreg;
> > > >   };
> > > >
> > > >   static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> > > >  @@ -162,11 +165,16 @@ static int imx_rproc_start(struct rproc
> > > > *rproc) struct device *dev = priv->dev;
> > > >           int ret;
> > > >
> > > >  + /* write entry point to program counter */
> > > >  + writel(rproc->bootaddr, priv->bootreg);
> > > >
> > > >
> > > > What happens on all the other IMX systems where this fix is not
> > > > needed? Will they continue to work properly?
> > > >
> > > > Yes, my bad, it is also needed for IMX6 (but even so i need to
> > > > study this topic more carefully), this should be applied
> > > > exclusively for imx7d for now, and if will be needed someone
> > > > with imx6 hardware to test on can extend this on imx6 also.
> > > >
> > > >
> > > >
> > > >
> > > >  +
> > > >           ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > > >                                    dcfg->src_mask,
> > > > dcfg->src_start); if (ret)
> > > >                   dev_err(dev, "Failed to enable M4!\n");
> > > >
> > > >  + dev_info(&rproc->dev, "Started from 0x%x\n",
> > > > rproc->bootaddr); +
> > > >           return ret;
> > > >   }
> > > >
> > > >  @@ -182,6 +190,9 @@ static int imx_rproc_stop(struct rproc
> > > > *rproc) if (ret)
> > > >                   dev_err(dev, "Failed to stop M4!\n");
> > > >
> > > >  + /* clear entry points */
> > > >  + writel(0, priv->bootreg);
> > > >  +
> > > >           return ret;
> > > >   }
> > > >
> > > >  @@ -243,7 +254,8 @@ static void *imx_rproc_da_to_va(struct
> > > > rproc *rproc, u64 da, int len) static const struct rproc_ops
> > > > imx_rproc_ops = { .start = imx_rproc_start,
> > > >           .stop = imx_rproc_stop,
> > > >  - .da_to_va = imx_rproc_da_to_va,
> > > >  + .da_to_va = imx_rproc_da_to_va,
> > > >  + .get_boot_addr = rproc_elf_get_boot_addr,
> > > >
> > > >
> > > > How is this useful? Sure it will set rproc->bootaddr in
> > > > rproc_fw_boot() but what good does that do when it is invariably
> > > > set again in imx_rproc_start() ?
> > > >
> > > > The priv->bootreg is the address where we are writing Entry
> > > > Point and it is fixed, 0x04 address is translated to
> > > > 0x00180004, so don't quite understand you we are writing
> > > > rproc->bootaddr into priv->bootreg, not wiseversa.
> > > >
> > >
> > > What is your reason to set ops->get_boot_addr ?  How does that
> > > help the work done in this patch?
> >
> > The reason is the following :
> >
> > remoteproc_core.c:
> > | rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> > | rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> >
> > remoteproc_internal.h
> > | static inline
> > | u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware
> > *fw) | {
> > |       if (rproc->ops->get_boot_addr)
> > |               return rproc->ops->get_boot_addr(rproc, fw);
> > |
> > |       return 0;
> > | }
> 
> And as I said above the value of rproc->bootaddr is set to
> priv->bootreg in imx_rproc_stop().  What am I missing?  More over
> imx_rproc_ops doesn't have a ->load() function and as such rproc_alloc
> will set it to rproc_elf_get_boot_addr()

Yes, you are totally correct, it is not required in this patch thank you
for pointing this out.

> 
> >
> > >
> > > >
> > > >   };
> > > >
> > > >   static int imx_rproc_addr_init(struct imx_rproc *priv,
> > > >  @@ -360,6 +372,8 @@ static int imx_rproc_probe(struct
> > > > platform_device *pdev) goto err_put_rproc;
> > > >           }
> > > >
> > > >  + priv->bootreg = imx_rproc_da_to_va(rproc, IMX_BOOT_PC,
> > > > sizeof(u32)); +
> > > >           /*
> > > >            * clk for M4 block including memory. Should be
> > > >            * enabled before .start for FW transfer.
> > > >  --
> > > >  2.25.1
> > > >
> >


^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2020-04-22  7:35 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04 14:26 [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Nikita Shubin
2020-03-04 14:26 ` [PATCH 2/2] remoteproc: imx_rproc: set pc on start Nikita Shubin
2020-03-05 16:16 ` [PATCH 1/2] remoteproc: imx_rproc: dummy kick method Mathieu Poirier
2020-03-05 17:29   ` nikita.shubin
2020-03-05 17:54     ` Mathieu Poirier
2020-03-05 18:07       ` nikita.shubin
2020-03-05 18:36         ` Mathieu Poirier
2020-03-05 18:46           ` nikita.shubin
2020-04-06 11:33 ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support nikita.shubin
2020-04-06 11:33   ` [PATCH v2 1/3] remoteproc: imx_rproc: set pc on start nikita.shubin
2020-04-14 16:45     ` Mathieu Poirier
2020-04-17 12:11       ` Nikita Shubin
2020-04-17 15:37         ` Mathieu Poirier
2020-04-17 15:46           ` Nikita Shubin
     [not found]       ` <45761587100993@mail.yandex.ru>
2020-04-17 17:01         ` Mathieu Poirier
2020-04-17 17:26           ` Nikita Shubin
2020-04-17 22:24             ` Mathieu Poirier
2020-04-22  7:35               ` Nikita Shubin
2020-04-06 11:33   ` [PATCH v2 2/3] remoteproc: imx_rproc: mailbox support nikita.shubin
2020-04-07  1:07     ` kbuild test robot
2020-04-14 17:20     ` Mathieu Poirier
2020-04-17  8:37       ` Nikita Shubin
2020-04-17 16:02         ` Mathieu Poirier
2020-04-14 17:36     ` Mathieu Poirier
2020-04-06 11:33   ` [PATCH v2 3/3] remoteproc: imx_rproc: memory regions nikita.shubin
2020-04-14 17:46     ` Mathieu Poirier
2020-04-15  2:42   ` [PATCH v2 0/3] remoteproc: imx_rproc: add virtio support Peng Fan
2020-04-15 16:26     ` Mathieu Poirier
2020-04-17  8:57     ` Nikita Shubin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).