* [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: 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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [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: 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
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
>
_______________________________________________
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 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: 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
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
> >
_______________________________________________
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 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: Ohad Ben-Cohen, Fabio Estevam, Nikita Shubin, Sascha Hauer,
linux-remoteproc, Linux Kernel Mailing List, Bjorn Andersson,
NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
linux-arm-kernel
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
> > >
>
_______________________________________________
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 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: Ohad Ben-Cohen, Fabio Estevam, Nikita Shubin, Sascha Hauer,
linux-remoteproc, Linux Kernel Mailing List, Bjorn Andersson,
NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
linux-arm-kernel
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
> > > >
> >
_______________________________________________
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
[parent not found: <45761587100993@mail.yandex.ru>]
* 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: 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
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
>
_______________________________________________
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 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: 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
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
> >
_______________________________________________
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 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: 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
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
> > >
>
_______________________________________________
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 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: 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
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
> > > >
> >
_______________________________________________
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
* [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: 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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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: Ohad Ben-Cohen, kbuild-all, Nikita Shubin, Sascha Hauer,
linux-remoteproc, linux-kernel, Bjorn Andersson, NXP Linux Team,
Pengutronix Kernel Team, Shawn Guo, Fabio Estevam,
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 --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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-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: 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
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
>
_______________________________________________
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: 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
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
> >
_______________________________________________
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-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: Ohad Ben-Cohen, Fabio Estevam, Nikita Shubin, Sascha Hauer,
linux-remoteproc, Linux Kernel Mailing List, Bjorn Andersson,
NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
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
> > >
>
_______________________________________________
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-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: 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
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
>
_______________________________________________
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
* [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: 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
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [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: 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
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
>
_______________________________________________
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 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, Shawn Guo, Sascha Hauer, linux-remoteproc,
linux-kernel, Bjorn Andersson, dl-linux-imx,
Pengutronix Kernel Team, Fabio Estevam, linux-arm-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
_______________________________________________
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 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: Ohad Ben-Cohen, nikita.shubin, Fabio Estevam, Sascha Hauer,
linux-remoteproc, linux-kernel, Bjorn Andersson, dl-linux-imx,
Pengutronix Kernel Team, Shawn Guo, 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
_______________________________________________
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 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, Shawn Guo, Sascha Hauer, linux-remoteproc,
linux-kernel, Bjorn Andersson, dl-linux-imx,
Pengutronix Kernel Team, Fabio Estevam, linux-arm-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
_______________________________________________
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