Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

Hi Bjorn,

thank you for review,

On 25.06.2017 22:41, Bjorn Andersson wrote:

On Wed 14 Jun 13:48 PDT 2017, Oleksij Rempel wrote:


From: Oleksij Rempel 

this driver was tested on NXP imx7d but should work on
imx6sx as well.
It will upload firmware to OCRAM, which shared memory between
Cortex A7 and Cortex M4, then turn M4 on.



This looks very generic, are there any IMX specific parts or is this the
"default" way of integrating a M4? Could we perhaps come up with a
common M4-rproc driver?


It is kind of generic. The differences are:
- platform specific clocks/reset/mbox configuration. So, maybe generic 
driver just should provide pre and post start/stop bindings?
- arrays for address translations. Naming seems not to be not really 
important. In my case:
 1) M4 own RAM for best performance. This can be requested immediately 
by rptoc driver.
 2) SRAM. Can be used by other parts of the system. Probably 
application specific, not good for mainline.
 3) limited range of DDR RAM. This should be dynamically allocated on 
request.


For (1) most drivers seem to have nearly identical code which IMO can be 
shared.



Even though it will be rather short, you need a DT binding document
defining the compatible and the few properties that you use.


Signed-off-by: Oleksij Rempel 
---
 drivers/remoteproc/Kconfig |   8 ++
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/imx_rproc.c | 264 +
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/remoteproc/imx_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..8d33bff4f530 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
tristate
depends on REMOTEPROC

+config IMX_REMOTEPROC


Please make an attempt to keep entries in the Kconfig and Makefile
alphabetically sorted.


+   tristate "IMX6/7 remoteproc support"
+   depends on SOC_IMX6SX || SOC_IMX7D
+   depends on REMOTEPROC
+   help
+ TODO, write some thing here
+ This can be either built-in or a loadable module.
+
 endif # REMOTEPROC

 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..d351c25cdb4e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,3 +19,4 @@ qcom_wcnss_pil-y  += qcom_wcnss.o
 qcom_wcnss_pil-y   += qcom_wcnss_iris.o
 obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
 obj-$(CONFIG_ST_SLIM_REMOTEPROC)   += st_slim_rproc.o
+obj-$(CONFIG_IMX_REMOTEPROC)   += imx_rproc.o
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
new file mode 100644
index ..6bef85237a27
--- /dev/null
+++ b/drivers/remoteproc/imx_rproc.c
@@ -0,0 +1,264 @@
+/*
+ * Oleksij Rempel 
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX7D_ENABLE_M4BIT(3)
+#define IMX7D_SW_M4P_RST   BIT(2)
+#define IMX7D_SW_M4C_RST   BIT(1)
+#define IMX7D_SW_M4C_NON_SCLR_RST  BIT(0)
+
+#define IMX7D_M4_RST_MASK  0xf
+
+
+#define IMX7D_RPROC_MEM_MAX2
+enum {
+   IMX7D_RPROC_IMEM,
+   IMX7D_RPROC_DMEM,
+};
+
+static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
+   [IMX7D_RPROC_IMEM]  = "imem",
+   [IMX7D_RPROC_DMEM]  = "dmem",
+};
+
+/**
+ * struct imx_rproc_mem - slim internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @size: Size of the memory region
+ */
+struct imx_rproc_mem {
+   void __iomem *cpu_addr;
+   phys_addr_t bus_addr;
+   size_t size;
+};
+
+struct imx_rproc_dcfg {
+   int offset;
+};


Unless you foresee this being expanded in the very near future I would
prefer that you just inline the offset in imx_rproc and put (void*)0xc
as .data in your of_match (typecast the of_device_get_match_data result
to unsigned long).


+
+struct imx_rproc {
+   struct device   *dev;
+   struct regmap   *regmap;
+   struct rproc*rproc;
+   const struct imx_rproc_dcfg *dcfg;
+   struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
+   struct clk  *clk;
+};
+
+static const 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

Hi Bjorn,

thank you for review,

On 25.06.2017 22:41, Bjorn Andersson wrote:

On Wed 14 Jun 13:48 PDT 2017, Oleksij Rempel wrote:


From: Oleksij Rempel 

this driver was tested on NXP imx7d but should work on
imx6sx as well.
It will upload firmware to OCRAM, which shared memory between
Cortex A7 and Cortex M4, then turn M4 on.



This looks very generic, are there any IMX specific parts or is this the
"default" way of integrating a M4? Could we perhaps come up with a
common M4-rproc driver?


It is kind of generic. The differences are:
- platform specific clocks/reset/mbox configuration. So, maybe generic 
driver just should provide pre and post start/stop bindings?
- arrays for address translations. Naming seems not to be not really 
important. In my case:
 1) M4 own RAM for best performance. This can be requested immediately 
by rptoc driver.
 2) SRAM. Can be used by other parts of the system. Probably 
application specific, not good for mainline.
 3) limited range of DDR RAM. This should be dynamically allocated on 
request.


For (1) most drivers seem to have nearly identical code which IMO can be 
shared.



Even though it will be rather short, you need a DT binding document
defining the compatible and the few properties that you use.


Signed-off-by: Oleksij Rempel 
---
 drivers/remoteproc/Kconfig |   8 ++
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/imx_rproc.c | 264 +
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/remoteproc/imx_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..8d33bff4f530 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
tristate
depends on REMOTEPROC

+config IMX_REMOTEPROC


Please make an attempt to keep entries in the Kconfig and Makefile
alphabetically sorted.


+   tristate "IMX6/7 remoteproc support"
+   depends on SOC_IMX6SX || SOC_IMX7D
+   depends on REMOTEPROC
+   help
+ TODO, write some thing here
+ This can be either built-in or a loadable module.
+
 endif # REMOTEPROC

 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..d351c25cdb4e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,3 +19,4 @@ qcom_wcnss_pil-y  += qcom_wcnss.o
 qcom_wcnss_pil-y   += qcom_wcnss_iris.o
 obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
 obj-$(CONFIG_ST_SLIM_REMOTEPROC)   += st_slim_rproc.o
+obj-$(CONFIG_IMX_REMOTEPROC)   += imx_rproc.o
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
new file mode 100644
index ..6bef85237a27
--- /dev/null
+++ b/drivers/remoteproc/imx_rproc.c
@@ -0,0 +1,264 @@
+/*
+ * Oleksij Rempel 
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX7D_ENABLE_M4BIT(3)
+#define IMX7D_SW_M4P_RST   BIT(2)
+#define IMX7D_SW_M4C_RST   BIT(1)
+#define IMX7D_SW_M4C_NON_SCLR_RST  BIT(0)
+
+#define IMX7D_M4_RST_MASK  0xf
+
+
+#define IMX7D_RPROC_MEM_MAX2
+enum {
+   IMX7D_RPROC_IMEM,
+   IMX7D_RPROC_DMEM,
+};
+
+static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
+   [IMX7D_RPROC_IMEM]  = "imem",
+   [IMX7D_RPROC_DMEM]  = "dmem",
+};
+
+/**
+ * struct imx_rproc_mem - slim internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @size: Size of the memory region
+ */
+struct imx_rproc_mem {
+   void __iomem *cpu_addr;
+   phys_addr_t bus_addr;
+   size_t size;
+};
+
+struct imx_rproc_dcfg {
+   int offset;
+};


Unless you foresee this being expanded in the very near future I would
prefer that you just inline the offset in imx_rproc and put (void*)0xc
as .data in your of_match (typecast the of_device_get_match_data result
to unsigned long).


+
+struct imx_rproc {
+   struct device   *dev;
+   struct regmap   *regmap;
+   struct rproc*rproc;
+   const struct imx_rproc_dcfg *dcfg;
+   struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
+   struct clk  *clk;
+};
+
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
+   .offset = 0xc,
+};
+

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Bjorn Andersson]

On Wed 14 Jun 13:48 PDT 2017, Oleksij Rempel wrote:

> From: Oleksij Rempel 
> 
> this driver was tested on NXP imx7d but should work on
> imx6sx as well.
> It will upload firmware to OCRAM, which shared memory between
> Cortex A7 and Cortex M4, then turn M4 on.
> 

This looks very generic, are there any IMX specific parts or is this the
"default" way of integrating a M4? Could we perhaps come up with a
common M4-rproc driver?


Even though it will be rather short, you need a DT binding document
defining the compatible and the few properties that you use.

> Signed-off-by: Oleksij Rempel 
> ---
>  drivers/remoteproc/Kconfig |   8 ++
>  drivers/remoteproc/Makefile|   1 +
>  drivers/remoteproc/imx_rproc.c | 264 
> +
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/remoteproc/imx_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index faad69a1a597..8d33bff4f530 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>   tristate
>   depends on REMOTEPROC
>  
> +config IMX_REMOTEPROC

Please make an attempt to keep entries in the Kconfig and Makefile
alphabetically sorted.

> + tristate "IMX6/7 remoteproc support"
> + depends on SOC_IMX6SX || SOC_IMX7D
> + depends on REMOTEPROC
> + help
> +   TODO, write some thing here
> +   This can be either built-in or a loadable module.
> +
>  endif # REMOTEPROC
>  
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ffc5e430df27..d351c25cdb4e 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
>  qcom_wcnss_pil-y += qcom_wcnss_iris.o
>  obj-$(CONFIG_ST_REMOTEPROC)  += st_remoteproc.o
>  obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> +obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> new file mode 100644
> index ..6bef85237a27
> --- /dev/null
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -0,0 +1,264 @@
> +/*
> + * Oleksij Rempel 
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define IMX7D_ENABLE_M4  BIT(3)
> +#define IMX7D_SW_M4P_RST BIT(2)
> +#define IMX7D_SW_M4C_RST BIT(1)
> +#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
> +
> +#define IMX7D_M4_RST_MASK0xf
> +
> +
> +#define IMX7D_RPROC_MEM_MAX  2
> +enum {
> + IMX7D_RPROC_IMEM,
> + IMX7D_RPROC_DMEM,
> +};
> +
> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> + [IMX7D_RPROC_IMEM]  = "imem",
> + [IMX7D_RPROC_DMEM]  = "dmem",
> +};
> +
> +/**
> + * struct imx_rproc_mem - slim internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @size: Size of the memory region
> + */
> +struct imx_rproc_mem {
> + void __iomem *cpu_addr;
> + phys_addr_t bus_addr;
> + size_t size;
> +};
> +
> +struct imx_rproc_dcfg {
> + int offset;
> +};

Unless you foresee this being expanded in the very near future I would
prefer that you just inline the offset in imx_rproc and put (void*)0xc
as .data in your of_match (typecast the of_device_get_match_data result
to unsigned long).

> +
> +struct imx_rproc {
> + struct device   *dev;
> + struct regmap   *regmap;
> + struct rproc*rproc;
> + const struct imx_rproc_dcfg *dcfg;
> + struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
> + struct clk  *clk;
> +};
> +
> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
> + .offset = 0xc,
> +};
> +
> +static int imx_rproc_start(struct rproc *rproc)
> +{
> + struct imx_rproc *priv = rproc->priv;
> + const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> + struct device *dev = priv->dev;
> + int ret;
> +
> + ret = clk_enable(priv->clk);
> + if (ret) {
> + dev_err(>dev, "Failed to enable clock\n");
> + return ret;
> + }
> +
> + ret = regmap_update_bits(priv->regmap, dcfg->offset,
> +

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Bjorn Andersson]

On Wed 14 Jun 13:48 PDT 2017, Oleksij Rempel wrote:

> From: Oleksij Rempel 
> 
> this driver was tested on NXP imx7d but should work on
> imx6sx as well.
> It will upload firmware to OCRAM, which shared memory between
> Cortex A7 and Cortex M4, then turn M4 on.
> 

This looks very generic, are there any IMX specific parts or is this the
"default" way of integrating a M4? Could we perhaps come up with a
common M4-rproc driver?


Even though it will be rather short, you need a DT binding document
defining the compatible and the few properties that you use.

> Signed-off-by: Oleksij Rempel 
> ---
>  drivers/remoteproc/Kconfig |   8 ++
>  drivers/remoteproc/Makefile|   1 +
>  drivers/remoteproc/imx_rproc.c | 264 
> +
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/remoteproc/imx_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index faad69a1a597..8d33bff4f530 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>   tristate
>   depends on REMOTEPROC
>  
> +config IMX_REMOTEPROC

Please make an attempt to keep entries in the Kconfig and Makefile
alphabetically sorted.

> + tristate "IMX6/7 remoteproc support"
> + depends on SOC_IMX6SX || SOC_IMX7D
> + depends on REMOTEPROC
> + help
> +   TODO, write some thing here
> +   This can be either built-in or a loadable module.
> +
>  endif # REMOTEPROC
>  
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ffc5e430df27..d351c25cdb4e 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
>  qcom_wcnss_pil-y += qcom_wcnss_iris.o
>  obj-$(CONFIG_ST_REMOTEPROC)  += st_remoteproc.o
>  obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> +obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> new file mode 100644
> index ..6bef85237a27
> --- /dev/null
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -0,0 +1,264 @@
> +/*
> + * Oleksij Rempel 
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define IMX7D_ENABLE_M4  BIT(3)
> +#define IMX7D_SW_M4P_RST BIT(2)
> +#define IMX7D_SW_M4C_RST BIT(1)
> +#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
> +
> +#define IMX7D_M4_RST_MASK0xf
> +
> +
> +#define IMX7D_RPROC_MEM_MAX  2
> +enum {
> + IMX7D_RPROC_IMEM,
> + IMX7D_RPROC_DMEM,
> +};
> +
> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> + [IMX7D_RPROC_IMEM]  = "imem",
> + [IMX7D_RPROC_DMEM]  = "dmem",
> +};
> +
> +/**
> + * struct imx_rproc_mem - slim internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @size: Size of the memory region
> + */
> +struct imx_rproc_mem {
> + void __iomem *cpu_addr;
> + phys_addr_t bus_addr;
> + size_t size;
> +};
> +
> +struct imx_rproc_dcfg {
> + int offset;
> +};

Unless you foresee this being expanded in the very near future I would
prefer that you just inline the offset in imx_rproc and put (void*)0xc
as .data in your of_match (typecast the of_device_get_match_data result
to unsigned long).

> +
> +struct imx_rproc {
> + struct device   *dev;
> + struct regmap   *regmap;
> + struct rproc*rproc;
> + const struct imx_rproc_dcfg *dcfg;
> + struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
> + struct clk  *clk;
> +};
> +
> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
> + .offset = 0xc,
> +};
> +
> +static int imx_rproc_start(struct rproc *rproc)
> +{
> + struct imx_rproc *priv = rproc->priv;
> + const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> + struct device *dev = priv->dev;
> + int ret;
> +
> + ret = clk_enable(priv->clk);
> + if (ret) {
> + dev_err(>dev, "Failed to enable clock\n");
> + return ret;
> + }
> +
> + ret = regmap_update_bits(priv->regmap, dcfg->offset,
> +  IMX7D_M4_RST_MASK,
> +  IMX7D_SW_M4C_RST | 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

On 06/23/2017 09:35 AM, Oleksij Rempel wrote:
> On Wed, Jun 21, 2017 at 04:09:26PM -0500, Suman Anna wrote:
>> Hi Oleksij,
>>
>> On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
>>> Hi,
>>>
>>> Thank you for your review!
>>>
>>> On 15.06.2017 21:01, Suman Anna wrote:
 Hi Oleksij,

 On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
> From: Oleksij Rempel 
>
> this driver was tested on NXP imx7d but should work on
> imx6sx as well.
> It will upload firmware to OCRAM, which shared memory between
> Cortex A7 and Cortex M4, then turn M4 on.

 Mostly looks fine, need to address few comments. I take it that you
 haven't added the binding since this is just an RFC.

> .
> +
> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> +[IMX7D_RPROC_IMEM]= "imem",
> +[IMX7D_RPROC_DMEM]= "dmem",
> +};

 Do you really need these to be globally defined? You only need them in
 the addr_init function, they can be made local to that function.
>>>
>>> I don't needed. At least not with my testing remote code.
>>> It is mostly copy/paste from existing drivers.
>>>
>>> But according to this page, there are multiple memory regions, with
>>> different mapping for data and code.
>>> http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
>>>
>>>
>>> "The Cortex-M4 CPU has two buses connected to the main interconnect
>>> (modified Harvard architecture). One bus is meant to fetch data (system
>>> bus) whereas the other bus is meant to fetch instructions (code bus). To
>>> get optimal performance, the program code should be located and linked
>>> for a region which is going to be fetched through the code bus, while
>>> the data area (e.g. bss or data section) should be located in a region
>>> which is fetched through the system bus.
>>
>> Yeah that's standard Cortex-M4 address/bus access architecture based on
>> memory addresses it sees, and the addresses are as per what the CPU
>> views them at.
>>
>>  There are multiple example
>>> linker files in the platform/devices/MCIMX7D/linker/ sub directory which
>>> can be used and/or modified. All example firmware below use the
>>> MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
>>> for data)."
>>>
>>> What is the proper way to implement it with remoteproc?
>>
>> So, TCML and TCMU looks to be internal memories within the Cortex-M4
>> subsystem from the link you shared. The rproc_da_to_va() is being used
>> today to provide the translations from the CPU device address (da) to
>> the kernel virtual address for the same memory (va) so that memcpy can
>> be used for loading the section data into those memories. The ioremap
>> from the A7 should be using the bus addresses.
> 
> I was reading linker and Make files provided by the git repo in the page
>  posted before. So far:
> - different app examples can use different linker configurations
>   (MCIMX7D_M4_ddr.ld, MCIMX7D_M4_ocram.ld or MCIMX7D_M4_tcm.ld)
> - only one configuration was used at the time.
> 
> I assume, to cover all this cases:
> - for each momory range should be created own DT node, with probably own
>   clock entry.  At leas *_tcm.ld and *_ocram.ld seems to fit in this pattern.
> 
> Should actually all possible variants be covered?

The TCML and TCMU are internal to your Cortex-M4 subsystem, so I expect
them to be defined within the corresponding node. This looks like the
minimum you would want to start with (given that it seems to be most
used in those examples). Obviously, the others depends on what you want
to support.

> 
> what is the best practice to proceed with shared ddr space? Should it be
> reserved-memory node in DT?

Yeah, reserved-memory node (CMA or carveout) in board files is what most
of the rproc drivers have been using so far. OCRAM and DDR usage may
vary from your use-case to use-case. I assume the OCRAM is the on-chip
RAM, so I take it that it will have its own DTS node (mmio-sram), and
reference it using a phandle in your remoteproc node. Usage will depend
on whether you need a fixed location or if you can allocate memory
dynamically.

> Should the translation map created in DeviceTree or in driver?

On TI devices, I have provided it through the driver.

regards
Suman

> 
>>
> +
> +/**
> + * struct imx_rproc_mem - slim internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @size: Size of the memory region
> + */
> +};
> +
> +struct imx_rproc_dcfg {
> +int offset;
> +};
> +
> +struct imx_rproc {
> +struct device*dev;
> +struct regmap*regmap;
> +struct rproc*rproc;
> +const struct imx_rproc_dcfg*dcfg;

> 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

On 06/23/2017 09:35 AM, Oleksij Rempel wrote:
> On Wed, Jun 21, 2017 at 04:09:26PM -0500, Suman Anna wrote:
>> Hi Oleksij,
>>
>> On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
>>> Hi,
>>>
>>> Thank you for your review!
>>>
>>> On 15.06.2017 21:01, Suman Anna wrote:
 Hi Oleksij,

 On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
> From: Oleksij Rempel 
>
> this driver was tested on NXP imx7d but should work on
> imx6sx as well.
> It will upload firmware to OCRAM, which shared memory between
> Cortex A7 and Cortex M4, then turn M4 on.

 Mostly looks fine, need to address few comments. I take it that you
 haven't added the binding since this is just an RFC.

> .
> +
> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> +[IMX7D_RPROC_IMEM]= "imem",
> +[IMX7D_RPROC_DMEM]= "dmem",
> +};

 Do you really need these to be globally defined? You only need them in
 the addr_init function, they can be made local to that function.
>>>
>>> I don't needed. At least not with my testing remote code.
>>> It is mostly copy/paste from existing drivers.
>>>
>>> But according to this page, there are multiple memory regions, with
>>> different mapping for data and code.
>>> http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
>>>
>>>
>>> "The Cortex-M4 CPU has two buses connected to the main interconnect
>>> (modified Harvard architecture). One bus is meant to fetch data (system
>>> bus) whereas the other bus is meant to fetch instructions (code bus). To
>>> get optimal performance, the program code should be located and linked
>>> for a region which is going to be fetched through the code bus, while
>>> the data area (e.g. bss or data section) should be located in a region
>>> which is fetched through the system bus.
>>
>> Yeah that's standard Cortex-M4 address/bus access architecture based on
>> memory addresses it sees, and the addresses are as per what the CPU
>> views them at.
>>
>>  There are multiple example
>>> linker files in the platform/devices/MCIMX7D/linker/ sub directory which
>>> can be used and/or modified. All example firmware below use the
>>> MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
>>> for data)."
>>>
>>> What is the proper way to implement it with remoteproc?
>>
>> So, TCML and TCMU looks to be internal memories within the Cortex-M4
>> subsystem from the link you shared. The rproc_da_to_va() is being used
>> today to provide the translations from the CPU device address (da) to
>> the kernel virtual address for the same memory (va) so that memcpy can
>> be used for loading the section data into those memories. The ioremap
>> from the A7 should be using the bus addresses.
> 
> I was reading linker and Make files provided by the git repo in the page
>  posted before. So far:
> - different app examples can use different linker configurations
>   (MCIMX7D_M4_ddr.ld, MCIMX7D_M4_ocram.ld or MCIMX7D_M4_tcm.ld)
> - only one configuration was used at the time.
> 
> I assume, to cover all this cases:
> - for each momory range should be created own DT node, with probably own
>   clock entry.  At leas *_tcm.ld and *_ocram.ld seems to fit in this pattern.
> 
> Should actually all possible variants be covered?

The TCML and TCMU are internal to your Cortex-M4 subsystem, so I expect
them to be defined within the corresponding node. This looks like the
minimum you would want to start with (given that it seems to be most
used in those examples). Obviously, the others depends on what you want
to support.

> 
> what is the best practice to proceed with shared ddr space? Should it be
> reserved-memory node in DT?

Yeah, reserved-memory node (CMA or carveout) in board files is what most
of the rproc drivers have been using so far. OCRAM and DDR usage may
vary from your use-case to use-case. I assume the OCRAM is the on-chip
RAM, so I take it that it will have its own DTS node (mmio-sram), and
reference it using a phandle in your remoteproc node. Usage will depend
on whether you need a fixed location or if you can allocate memory
dynamically.

> Should the translation map created in DeviceTree or in driver?

On TI devices, I have provided it through the driver.

regards
Suman

> 
>>
> +
> +/**
> + * struct imx_rproc_mem - slim internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @size: Size of the memory region
> + */
> +};
> +
> +struct imx_rproc_dcfg {
> +int offset;
> +};
> +
> +struct imx_rproc {
> +struct device*dev;
> +struct regmap*regmap;
> +struct rproc*rproc;
> +const struct imx_rproc_dcfg*dcfg;

> 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

On Wed, Jun 21, 2017 at 04:09:26PM -0500, Suman Anna wrote:
> Hi Oleksij,
> 
> On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
> > Hi,
> > 
> > Thank you for your review!
> > 
> > On 15.06.2017 21:01, Suman Anna wrote:
> >> Hi Oleksij,
> >>
> >> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
> >>> From: Oleksij Rempel 
> >>>
> >>> this driver was tested on NXP imx7d but should work on
> >>> imx6sx as well.
> >>> It will upload firmware to OCRAM, which shared memory between
> >>> Cortex A7 and Cortex M4, then turn M4 on.
> >>
> >> Mostly looks fine, need to address few comments. I take it that you
> >> haven't added the binding since this is just an RFC.
> >>
.
> >>> +
> >>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> >>> +[IMX7D_RPROC_IMEM]= "imem",
> >>> +[IMX7D_RPROC_DMEM]= "dmem",
> >>> +};
> >>
> >> Do you really need these to be globally defined? You only need them in
> >> the addr_init function, they can be made local to that function.
> > 
> > I don't needed. At least not with my testing remote code.
> > It is mostly copy/paste from existing drivers.
> > 
> > But according to this page, there are multiple memory regions, with
> > different mapping for data and code.
> > http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
> > 
> > 
> > "The Cortex-M4 CPU has two buses connected to the main interconnect
> > (modified Harvard architecture). One bus is meant to fetch data (system
> > bus) whereas the other bus is meant to fetch instructions (code bus). To
> > get optimal performance, the program code should be located and linked
> > for a region which is going to be fetched through the code bus, while
> > the data area (e.g. bss or data section) should be located in a region
> > which is fetched through the system bus.
> 
> Yeah that's standard Cortex-M4 address/bus access architecture based on
> memory addresses it sees, and the addresses are as per what the CPU
> views them at.
> 
>  There are multiple example
> > linker files in the platform/devices/MCIMX7D/linker/ sub directory which
> > can be used and/or modified. All example firmware below use the
> > MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
> > for data)."
> > 
> > What is the proper way to implement it with remoteproc?
> 
> So, TCML and TCMU looks to be internal memories within the Cortex-M4
> subsystem from the link you shared. The rproc_da_to_va() is being used
> today to provide the translations from the CPU device address (da) to
> the kernel virtual address for the same memory (va) so that memcpy can
> be used for loading the section data into those memories. The ioremap
> from the A7 should be using the bus addresses.

I was reading linker and Make files provided by the git repo in the page
 posted before. So far:
- different app examples can use different linker configurations
  (MCIMX7D_M4_ddr.ld, MCIMX7D_M4_ocram.ld or MCIMX7D_M4_tcm.ld)
- only one configuration was used at the time.

I assume, to cover all this cases:
- for each momory range should be created own DT node, with probably own
  clock entry.  At leas *_tcm.ld and *_ocram.ld seems to fit in this pattern.

Should actually all possible variants be covered?

what is the best practice to proceed with shared ddr space? Should it be
reserved-memory node in DT?
Should the translation map created in DeviceTree or in driver?

> 
> >>> +
> >>> +/**
> >>> + * struct imx_rproc_mem - slim internal memory structure
> >>> + * @cpu_addr: MPU virtual address of the memory region
> >>> + * @bus_addr: Bus address used to access the memory region
> >>> + * @size: Size of the memory region
> >>> + */
> >>> +};
> >>> +
> >>> +struct imx_rproc_dcfg {
> >>> +int offset;
> >>> +};
> >>> +
> >>> +struct imx_rproc {
> >>> +struct device*dev;
> >>> +struct regmap*regmap;
> >>> +struct rproc*rproc;
> >>> +const struct imx_rproc_dcfg*dcfg;
> >>

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

On Wed, Jun 21, 2017 at 04:09:26PM -0500, Suman Anna wrote:
> Hi Oleksij,
> 
> On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
> > Hi,
> > 
> > Thank you for your review!
> > 
> > On 15.06.2017 21:01, Suman Anna wrote:
> >> Hi Oleksij,
> >>
> >> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
> >>> From: Oleksij Rempel 
> >>>
> >>> this driver was tested on NXP imx7d but should work on
> >>> imx6sx as well.
> >>> It will upload firmware to OCRAM, which shared memory between
> >>> Cortex A7 and Cortex M4, then turn M4 on.
> >>
> >> Mostly looks fine, need to address few comments. I take it that you
> >> haven't added the binding since this is just an RFC.
> >>
.
> >>> +
> >>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> >>> +[IMX7D_RPROC_IMEM]= "imem",
> >>> +[IMX7D_RPROC_DMEM]= "dmem",
> >>> +};
> >>
> >> Do you really need these to be globally defined? You only need them in
> >> the addr_init function, they can be made local to that function.
> > 
> > I don't needed. At least not with my testing remote code.
> > It is mostly copy/paste from existing drivers.
> > 
> > But according to this page, there are multiple memory regions, with
> > different mapping for data and code.
> > http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
> > 
> > 
> > "The Cortex-M4 CPU has two buses connected to the main interconnect
> > (modified Harvard architecture). One bus is meant to fetch data (system
> > bus) whereas the other bus is meant to fetch instructions (code bus). To
> > get optimal performance, the program code should be located and linked
> > for a region which is going to be fetched through the code bus, while
> > the data area (e.g. bss or data section) should be located in a region
> > which is fetched through the system bus.
> 
> Yeah that's standard Cortex-M4 address/bus access architecture based on
> memory addresses it sees, and the addresses are as per what the CPU
> views them at.
> 
>  There are multiple example
> > linker files in the platform/devices/MCIMX7D/linker/ sub directory which
> > can be used and/or modified. All example firmware below use the
> > MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
> > for data)."
> > 
> > What is the proper way to implement it with remoteproc?
> 
> So, TCML and TCMU looks to be internal memories within the Cortex-M4
> subsystem from the link you shared. The rproc_da_to_va() is being used
> today to provide the translations from the CPU device address (da) to
> the kernel virtual address for the same memory (va) so that memcpy can
> be used for loading the section data into those memories. The ioremap
> from the A7 should be using the bus addresses.

I was reading linker and Make files provided by the git repo in the page
 posted before. So far:
- different app examples can use different linker configurations
  (MCIMX7D_M4_ddr.ld, MCIMX7D_M4_ocram.ld or MCIMX7D_M4_tcm.ld)
- only one configuration was used at the time.

I assume, to cover all this cases:
- for each momory range should be created own DT node, with probably own
  clock entry.  At leas *_tcm.ld and *_ocram.ld seems to fit in this pattern.

Should actually all possible variants be covered?

what is the best practice to proceed with shared ddr space? Should it be
reserved-memory node in DT?
Should the translation map created in DeviceTree or in driver?

> 
> >>> +
> >>> +/**
> >>> + * struct imx_rproc_mem - slim internal memory structure
> >>> + * @cpu_addr: MPU virtual address of the memory region
> >>> + * @bus_addr: Bus address used to access the memory region
> >>> + * @size: Size of the memory region
> >>> + */
> >>> +};
> >>> +
> >>> +struct imx_rproc_dcfg {
> >>> +int offset;
> >>> +};
> >>> +
> >>> +struct imx_rproc {
> >>> +struct device*dev;
> >>> +struct regmap*regmap;
> >>> +struct rproc*rproc;
> >>> +const struct imx_rproc_dcfg*dcfg;
> >>

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

Hi Oleksij,

On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
> Hi,
> 
> Thank you for your review!
> 
> On 15.06.2017 21:01, Suman Anna wrote:
>> Hi Oleksij,
>>
>> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>>> From: Oleksij Rempel 
>>>
>>> this driver was tested on NXP imx7d but should work on
>>> imx6sx as well.
>>> It will upload firmware to OCRAM, which shared memory between
>>> Cortex A7 and Cortex M4, then turn M4 on.
>>
>> Mostly looks fine, need to address few comments. I take it that you
>> haven't added the binding since this is just an RFC.
>>
>>>
>>> Signed-off-by: Oleksij Rempel 
>>> ---
>>>   drivers/remoteproc/Kconfig |   8 ++
>>>   drivers/remoteproc/Makefile|   1 +
>>>   drivers/remoteproc/imx_rproc.c | 264
>>> +
>>>   3 files changed, 273 insertions(+)
>>>   create mode 100644 drivers/remoteproc/imx_rproc.c
>>>
>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>> index faad69a1a597..8d33bff4f530 100644
>>> --- a/drivers/remoteproc/Kconfig
>>> +++ b/drivers/remoteproc/Kconfig
>>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>>   tristate
>>>   depends on REMOTEPROC
>>>   +config IMX_REMOTEPROC
>>> +tristate "IMX6/7 remoteproc support"
>>> +depends on SOC_IMX6SX || SOC_IMX7D
>>> +depends on REMOTEPROC
>>> +help
>>> +  TODO, write some thing here
>>> +  This can be either built-in or a loadable module.
>>> +
>>>   endif # REMOTEPROC
>>> endmenu
>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>>> index ffc5e430df27..d351c25cdb4e 100644
>>> --- a/drivers/remoteproc/Makefile
>>> +++ b/drivers/remoteproc/Makefile
>>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
>>>   qcom_wcnss_pil-y+= qcom_wcnss_iris.o
>>>   obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
>>>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)+= st_slim_rproc.o
>>> +obj-$(CONFIG_IMX_REMOTEPROC)+= imx_rproc.o
>>> diff --git a/drivers/remoteproc/imx_rproc.c
>>> b/drivers/remoteproc/imx_rproc.c
>>> new file mode 100644
>>> index ..6bef85237a27
>>> --- /dev/null
>>> +++ b/drivers/remoteproc/imx_rproc.c
>>> @@ -0,0 +1,264 @@
>>> +/*
>>> + * Oleksij Rempel 
>>
>> Please add a blank line here.
>>
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#define IMX7D_ENABLE_M4BIT(3)
>>> +#define IMX7D_SW_M4P_RSTBIT(2)
>>> +#define IMX7D_SW_M4C_RSTBIT(1)
>>> +#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
>>> +
>>> +#define IMX7D_M4_RST_MASK0xf
>>> +
>>> +
>>> +#define IMX7D_RPROC_MEM_MAX2
>>> +enum {
>>> +IMX7D_RPROC_IMEM,
>>> +IMX7D_RPROC_DMEM,
>>> +};
>>> +
>>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>>> +[IMX7D_RPROC_IMEM]= "imem",
>>> +[IMX7D_RPROC_DMEM]= "dmem",
>>> +};
>>
>> Do you really need these to be globally defined? You only need them in
>> the addr_init function, they can be made local to that function.
> 
> I don't needed. At least not with my testing remote code.
> It is mostly copy/paste from existing drivers.
> 
> But according to this page, there are multiple memory regions, with
> different mapping for data and code.
> http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
> 
> 
> "The Cortex-M4 CPU has two buses connected to the main interconnect
> (modified Harvard architecture). One bus is meant to fetch data (system
> bus) whereas the other bus is meant to fetch instructions (code bus). To
> get optimal performance, the program code should be located and linked
> for a region which is going to be fetched through the code bus, while
> the data area (e.g. bss or data section) should be located in a region
> which is fetched through the system bus.

Yeah that's standard Cortex-M4 address/bus access architecture based on
memory addresses it sees, and the addresses are as per what the CPU
views them at.

 There are multiple example
> linker files in the platform/devices/MCIMX7D/linker/ sub directory which
> can be used and/or modified. All example firmware below use the
> MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
> for data)."
> 
> What is the proper way to implement it with remoteproc?

So, TCML and TCMU looks to 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

Hi Oleksij,

On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
> Hi,
> 
> Thank you for your review!
> 
> On 15.06.2017 21:01, Suman Anna wrote:
>> Hi Oleksij,
>>
>> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>>> From: Oleksij Rempel 
>>>
>>> this driver was tested on NXP imx7d but should work on
>>> imx6sx as well.
>>> It will upload firmware to OCRAM, which shared memory between
>>> Cortex A7 and Cortex M4, then turn M4 on.
>>
>> Mostly looks fine, need to address few comments. I take it that you
>> haven't added the binding since this is just an RFC.
>>
>>>
>>> Signed-off-by: Oleksij Rempel 
>>> ---
>>>   drivers/remoteproc/Kconfig |   8 ++
>>>   drivers/remoteproc/Makefile|   1 +
>>>   drivers/remoteproc/imx_rproc.c | 264
>>> +
>>>   3 files changed, 273 insertions(+)
>>>   create mode 100644 drivers/remoteproc/imx_rproc.c
>>>
>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>> index faad69a1a597..8d33bff4f530 100644
>>> --- a/drivers/remoteproc/Kconfig
>>> +++ b/drivers/remoteproc/Kconfig
>>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>>   tristate
>>>   depends on REMOTEPROC
>>>   +config IMX_REMOTEPROC
>>> +tristate "IMX6/7 remoteproc support"
>>> +depends on SOC_IMX6SX || SOC_IMX7D
>>> +depends on REMOTEPROC
>>> +help
>>> +  TODO, write some thing here
>>> +  This can be either built-in or a loadable module.
>>> +
>>>   endif # REMOTEPROC
>>> endmenu
>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>>> index ffc5e430df27..d351c25cdb4e 100644
>>> --- a/drivers/remoteproc/Makefile
>>> +++ b/drivers/remoteproc/Makefile
>>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
>>>   qcom_wcnss_pil-y+= qcom_wcnss_iris.o
>>>   obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
>>>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)+= st_slim_rproc.o
>>> +obj-$(CONFIG_IMX_REMOTEPROC)+= imx_rproc.o
>>> diff --git a/drivers/remoteproc/imx_rproc.c
>>> b/drivers/remoteproc/imx_rproc.c
>>> new file mode 100644
>>> index ..6bef85237a27
>>> --- /dev/null
>>> +++ b/drivers/remoteproc/imx_rproc.c
>>> @@ -0,0 +1,264 @@
>>> +/*
>>> + * Oleksij Rempel 
>>
>> Please add a blank line here.
>>
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#define IMX7D_ENABLE_M4BIT(3)
>>> +#define IMX7D_SW_M4P_RSTBIT(2)
>>> +#define IMX7D_SW_M4C_RSTBIT(1)
>>> +#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
>>> +
>>> +#define IMX7D_M4_RST_MASK0xf
>>> +
>>> +
>>> +#define IMX7D_RPROC_MEM_MAX2
>>> +enum {
>>> +IMX7D_RPROC_IMEM,
>>> +IMX7D_RPROC_DMEM,
>>> +};
>>> +
>>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>>> +[IMX7D_RPROC_IMEM]= "imem",
>>> +[IMX7D_RPROC_DMEM]= "dmem",
>>> +};
>>
>> Do you really need these to be globally defined? You only need them in
>> the addr_init function, they can be made local to that function.
> 
> I don't needed. At least not with my testing remote code.
> It is mostly copy/paste from existing drivers.
> 
> But according to this page, there are multiple memory regions, with
> different mapping for data and code.
> http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
> 
> 
> "The Cortex-M4 CPU has two buses connected to the main interconnect
> (modified Harvard architecture). One bus is meant to fetch data (system
> bus) whereas the other bus is meant to fetch instructions (code bus). To
> get optimal performance, the program code should be located and linked
> for a region which is going to be fetched through the code bus, while
> the data area (e.g. bss or data section) should be located in a region
> which is fetched through the system bus.

Yeah that's standard Cortex-M4 address/bus access architecture based on
memory addresses it sees, and the addresses are as per what the CPU
views them at.

 There are multiple example
> linker files in the platform/devices/MCIMX7D/linker/ sub directory which
> can be used and/or modified. All example firmware below use the
> MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
> for data)."
> 
> What is the proper way to implement it with remoteproc?

So, TCML and TCMU looks to be internal memories within the Cortex-M4
subsystem from the link you 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

Hi,

Thank you for your review!

On 15.06.2017 21:01, Suman Anna wrote:

Hi Oleksij,

On 06/14/2017 03:48 PM, Oleksij Rempel wrote:

From: Oleksij Rempel 

this driver was tested on NXP imx7d but should work on
imx6sx as well.
It will upload firmware to OCRAM, which shared memory between
Cortex A7 and Cortex M4, then turn M4 on.


Mostly looks fine, need to address few comments. I take it that you
haven't added the binding since this is just an RFC.



Signed-off-by: Oleksij Rempel 
---
  drivers/remoteproc/Kconfig |   8 ++
  drivers/remoteproc/Makefile|   1 +
  drivers/remoteproc/imx_rproc.c | 264
+
  3 files changed, 273 insertions(+)
  create mode 100644 drivers/remoteproc/imx_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..8d33bff4f530 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
  tristate
  depends on REMOTEPROC
  +config IMX_REMOTEPROC
+tristate "IMX6/7 remoteproc support"
+depends on SOC_IMX6SX || SOC_IMX7D
+depends on REMOTEPROC
+help
+  TODO, write some thing here
+  This can be either built-in or a loadable module.
+
  endif # REMOTEPROC
endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..d351c25cdb4e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
  qcom_wcnss_pil-y+= qcom_wcnss_iris.o
  obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
  obj-$(CONFIG_ST_SLIM_REMOTEPROC)+= st_slim_rproc.o
+obj-$(CONFIG_IMX_REMOTEPROC)+= imx_rproc.o
diff --git a/drivers/remoteproc/imx_rproc.c
b/drivers/remoteproc/imx_rproc.c
new file mode 100644
index ..6bef85237a27
--- /dev/null
+++ b/drivers/remoteproc/imx_rproc.c
@@ -0,0 +1,264 @@
+/*
+ * Oleksij Rempel 


Please add a blank line here.


+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX7D_ENABLE_M4BIT(3)
+#define IMX7D_SW_M4P_RSTBIT(2)
+#define IMX7D_SW_M4C_RSTBIT(1)
+#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
+
+#define IMX7D_M4_RST_MASK0xf
+
+
+#define IMX7D_RPROC_MEM_MAX2
+enum {
+IMX7D_RPROC_IMEM,
+IMX7D_RPROC_DMEM,
+};
+
+static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
+[IMX7D_RPROC_IMEM]= "imem",
+[IMX7D_RPROC_DMEM]= "dmem",
+};


Do you really need these to be globally defined? You only need them in
the addr_init function, they can be made local to that function.


I don't needed. At least not with my testing remote code.
It is mostly copy/paste from existing drivers.

But according to this page, there are multiple memory regions, with 
different mapping for data and code.

http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas

"The Cortex-M4 CPU has two buses connected to the main interconnect 
(modified Harvard architecture). One bus is meant to fetch data (system 
bus) whereas the other bus is meant to fetch instructions (code bus). To 
get optimal performance, the program code should be located and linked 
for a region which is going to be fetched through the code bus, while 
the data area (e.g. bss or data section) should be located in a region 
which is fetched through the system bus. There are multiple example 
linker files in the platform/devices/MCIMX7D/linker/ sub directory which 
can be used and/or modified. All example firmware below use the 
MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region 
for data)."


What is the proper way to implement it with remoteproc?


+
+/**
+ * struct imx_rproc_mem - slim internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @size: Size of the memory region
+ */
+struct imx_rproc_mem {
+void __iomem *cpu_addr;
+phys_addr_t bus_addr;


Are the rproc-view of these regions same as the bus addresses or
do they have their own local addresses? If latter, images that are built
for those will be using those addresses in which case you need some
offset calculation in the da_to_va ops?


+size_t size;
+};
+
+struct imx_rproc_dcfg {
+int offset;
+};
+
+struct imx_rproc {
+struct device

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

Hi,

Thank you for your review!

On 15.06.2017 21:01, Suman Anna wrote:

Hi Oleksij,

On 06/14/2017 03:48 PM, Oleksij Rempel wrote:

From: Oleksij Rempel 

this driver was tested on NXP imx7d but should work on
imx6sx as well.
It will upload firmware to OCRAM, which shared memory between
Cortex A7 and Cortex M4, then turn M4 on.


Mostly looks fine, need to address few comments. I take it that you
haven't added the binding since this is just an RFC.



Signed-off-by: Oleksij Rempel 
---
  drivers/remoteproc/Kconfig |   8 ++
  drivers/remoteproc/Makefile|   1 +
  drivers/remoteproc/imx_rproc.c | 264
+
  3 files changed, 273 insertions(+)
  create mode 100644 drivers/remoteproc/imx_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..8d33bff4f530 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
  tristate
  depends on REMOTEPROC
  +config IMX_REMOTEPROC
+tristate "IMX6/7 remoteproc support"
+depends on SOC_IMX6SX || SOC_IMX7D
+depends on REMOTEPROC
+help
+  TODO, write some thing here
+  This can be either built-in or a loadable module.
+
  endif # REMOTEPROC
endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..d351c25cdb4e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
  qcom_wcnss_pil-y+= qcom_wcnss_iris.o
  obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
  obj-$(CONFIG_ST_SLIM_REMOTEPROC)+= st_slim_rproc.o
+obj-$(CONFIG_IMX_REMOTEPROC)+= imx_rproc.o
diff --git a/drivers/remoteproc/imx_rproc.c
b/drivers/remoteproc/imx_rproc.c
new file mode 100644
index ..6bef85237a27
--- /dev/null
+++ b/drivers/remoteproc/imx_rproc.c
@@ -0,0 +1,264 @@
+/*
+ * Oleksij Rempel 


Please add a blank line here.


+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX7D_ENABLE_M4BIT(3)
+#define IMX7D_SW_M4P_RSTBIT(2)
+#define IMX7D_SW_M4C_RSTBIT(1)
+#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
+
+#define IMX7D_M4_RST_MASK0xf
+
+
+#define IMX7D_RPROC_MEM_MAX2
+enum {
+IMX7D_RPROC_IMEM,
+IMX7D_RPROC_DMEM,
+};
+
+static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
+[IMX7D_RPROC_IMEM]= "imem",
+[IMX7D_RPROC_DMEM]= "dmem",
+};


Do you really need these to be globally defined? You only need them in
the addr_init function, they can be made local to that function.


I don't needed. At least not with my testing remote code.
It is mostly copy/paste from existing drivers.

But according to this page, there are multiple memory regions, with 
different mapping for data and code.

http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas

"The Cortex-M4 CPU has two buses connected to the main interconnect 
(modified Harvard architecture). One bus is meant to fetch data (system 
bus) whereas the other bus is meant to fetch instructions (code bus). To 
get optimal performance, the program code should be located and linked 
for a region which is going to be fetched through the code bus, while 
the data area (e.g. bss or data section) should be located in a region 
which is fetched through the system bus. There are multiple example 
linker files in the platform/devices/MCIMX7D/linker/ sub directory which 
can be used and/or modified. All example firmware below use the 
MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region 
for data)."


What is the proper way to implement it with remoteproc?


+
+/**
+ * struct imx_rproc_mem - slim internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @size: Size of the memory region
+ */
+struct imx_rproc_mem {
+void __iomem *cpu_addr;
+phys_addr_t bus_addr;


Are the rproc-view of these regions same as the bus addresses or
do they have their own local addresses? If latter, images that are built
for those will be using those addresses in which case you need some
offset calculation in the da_to_va ops?


+size_t size;
+};
+
+struct imx_rproc_dcfg {
+int offset;
+};
+
+struct imx_rproc {
+struct device*dev;
+struct regmap*regmap;
+struct rproc   

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

On 06/15/2017 02:01 PM, Suman Anna wrote:
> Hi Oleksij,
> 
> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>> From: Oleksij Rempel 
>>
>> this driver was tested on NXP imx7d but should work on
>> imx6sx as well.
>> It will upload firmware to OCRAM, which shared memory between
>> Cortex A7 and Cortex M4, then turn M4 on.
> 
> Mostly looks fine, need to address few comments. I take it that you
> haven't added the binding since this is just an RFC.
> 
>>
>> Signed-off-by: Oleksij Rempel 
>> ---
>>   drivers/remoteproc/Kconfig |   8 ++
>>   drivers/remoteproc/Makefile|   1 +
>>   drivers/remoteproc/imx_rproc.c | 264
>> +
>>   3 files changed, 273 insertions(+)
>>   create mode 100644 drivers/remoteproc/imx_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index faad69a1a597..8d33bff4f530 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>   tristate
>>   depends on REMOTEPROC
>>   +config IMX_REMOTEPROC
>> +tristate "IMX6/7 remoteproc support"
>> +depends on SOC_IMX6SX || SOC_IMX7D
>> +depends on REMOTEPROC
>> +help
>> +  TODO, write some thing here
>> +  This can be either built-in or a loadable module.
>> +
>>   endif # REMOTEPROC
>> endmenu
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index ffc5e430df27..d351c25cdb4e 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
>>   qcom_wcnss_pil-y+= qcom_wcnss_iris.o
>>   obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
>>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)+= st_slim_rproc.o
>> +obj-$(CONFIG_IMX_REMOTEPROC)+= imx_rproc.o
>> diff --git a/drivers/remoteproc/imx_rproc.c
>> b/drivers/remoteproc/imx_rproc.c
>> new file mode 100644
>> index ..6bef85237a27
>> --- /dev/null
>> +++ b/drivers/remoteproc/imx_rproc.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Oleksij Rempel 
> 
> Please add a blank line here.
> 
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define IMX7D_ENABLE_M4BIT(3)
>> +#define IMX7D_SW_M4P_RSTBIT(2)
>> +#define IMX7D_SW_M4C_RSTBIT(1)
>> +#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
>> +
>> +#define IMX7D_M4_RST_MASK0xf
>> +
>> +
>> +#define IMX7D_RPROC_MEM_MAX2
>> +enum {
>> +IMX7D_RPROC_IMEM,
>> +IMX7D_RPROC_DMEM,
>> +};
>> +
>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>> +[IMX7D_RPROC_IMEM]= "imem",
>> +[IMX7D_RPROC_DMEM]= "dmem",
>> +};
> 
> Do you really need these to be globally defined? You only need them in
> the addr_init function, they can be made local to that function.
> 
>> +
>> +/**
>> + * struct imx_rproc_mem - slim internal memory structure
>> + * @cpu_addr: MPU virtual address of the memory region
>> + * @bus_addr: Bus address used to access the memory region
>> + * @size: Size of the memory region
>> + */
>> +struct imx_rproc_mem {
>> +void __iomem *cpu_addr;
>> +phys_addr_t bus_addr;
> 
> Are the rproc-view of these regions same as the bus addresses or
> do they have their own local addresses? If latter, images that are built
> for those will be using those addresses in which case you need some
> offset calculation in the da_to_va ops?
> 
>> +size_t size;
>> +};
>> +
>> +struct imx_rproc_dcfg {
>> +int offset;
>> +};
>> +
>> +struct imx_rproc {
>> +struct device*dev;
>> +struct regmap*regmap;
>> +struct rproc*rproc;
>> +const struct imx_rproc_dcfg*dcfg;
> 
> No need of aligning the variables, you can get rid of the additional
> whitespaces and maintain consistent style.
> 
>> +struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>> +struct clk*clk;
>> +};
>> +
>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>> +.offset = 0xc,
>> +};
>> +
>> +static int imx_rproc_start(struct rproc *rproc)
>> +{
>> +struct imx_rproc *priv = rproc->priv;
>> +const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +struct device *dev = priv->dev;
>> +int ret;
>> +
>> +ret = clk_enable(priv->clk);
>> +if (ret) {
>> +dev_err(>dev, "Failed to 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

On 06/15/2017 02:01 PM, Suman Anna wrote:
> Hi Oleksij,
> 
> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>> From: Oleksij Rempel 
>>
>> this driver was tested on NXP imx7d but should work on
>> imx6sx as well.
>> It will upload firmware to OCRAM, which shared memory between
>> Cortex A7 and Cortex M4, then turn M4 on.
> 
> Mostly looks fine, need to address few comments. I take it that you
> haven't added the binding since this is just an RFC.
> 
>>
>> Signed-off-by: Oleksij Rempel 
>> ---
>>   drivers/remoteproc/Kconfig |   8 ++
>>   drivers/remoteproc/Makefile|   1 +
>>   drivers/remoteproc/imx_rproc.c | 264
>> +
>>   3 files changed, 273 insertions(+)
>>   create mode 100644 drivers/remoteproc/imx_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index faad69a1a597..8d33bff4f530 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>   tristate
>>   depends on REMOTEPROC
>>   +config IMX_REMOTEPROC
>> +tristate "IMX6/7 remoteproc support"
>> +depends on SOC_IMX6SX || SOC_IMX7D
>> +depends on REMOTEPROC
>> +help
>> +  TODO, write some thing here
>> +  This can be either built-in or a loadable module.
>> +
>>   endif # REMOTEPROC
>> endmenu
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index ffc5e430df27..d351c25cdb4e 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y+= qcom_wcnss.o
>>   qcom_wcnss_pil-y+= qcom_wcnss_iris.o
>>   obj-$(CONFIG_ST_REMOTEPROC)+= st_remoteproc.o
>>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)+= st_slim_rproc.o
>> +obj-$(CONFIG_IMX_REMOTEPROC)+= imx_rproc.o
>> diff --git a/drivers/remoteproc/imx_rproc.c
>> b/drivers/remoteproc/imx_rproc.c
>> new file mode 100644
>> index ..6bef85237a27
>> --- /dev/null
>> +++ b/drivers/remoteproc/imx_rproc.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Oleksij Rempel 
> 
> Please add a blank line here.
> 
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define IMX7D_ENABLE_M4BIT(3)
>> +#define IMX7D_SW_M4P_RSTBIT(2)
>> +#define IMX7D_SW_M4C_RSTBIT(1)
>> +#define IMX7D_SW_M4C_NON_SCLR_RSTBIT(0)
>> +
>> +#define IMX7D_M4_RST_MASK0xf
>> +
>> +
>> +#define IMX7D_RPROC_MEM_MAX2
>> +enum {
>> +IMX7D_RPROC_IMEM,
>> +IMX7D_RPROC_DMEM,
>> +};
>> +
>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>> +[IMX7D_RPROC_IMEM]= "imem",
>> +[IMX7D_RPROC_DMEM]= "dmem",
>> +};
> 
> Do you really need these to be globally defined? You only need them in
> the addr_init function, they can be made local to that function.
> 
>> +
>> +/**
>> + * struct imx_rproc_mem - slim internal memory structure
>> + * @cpu_addr: MPU virtual address of the memory region
>> + * @bus_addr: Bus address used to access the memory region
>> + * @size: Size of the memory region
>> + */
>> +struct imx_rproc_mem {
>> +void __iomem *cpu_addr;
>> +phys_addr_t bus_addr;
> 
> Are the rproc-view of these regions same as the bus addresses or
> do they have their own local addresses? If latter, images that are built
> for those will be using those addresses in which case you need some
> offset calculation in the da_to_va ops?
> 
>> +size_t size;
>> +};
>> +
>> +struct imx_rproc_dcfg {
>> +int offset;
>> +};
>> +
>> +struct imx_rproc {
>> +struct device*dev;
>> +struct regmap*regmap;
>> +struct rproc*rproc;
>> +const struct imx_rproc_dcfg*dcfg;
> 
> No need of aligning the variables, you can get rid of the additional
> whitespaces and maintain consistent style.
> 
>> +struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>> +struct clk*clk;
>> +};
>> +
>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>> +.offset = 0xc,
>> +};
>> +
>> +static int imx_rproc_start(struct rproc *rproc)
>> +{
>> +struct imx_rproc *priv = rproc->priv;
>> +const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +struct device *dev = priv->dev;
>> +int ret;
>> +
>> +ret = clk_enable(priv->clk);
>> +if (ret) {
>> +dev_err(>dev, "Failed to enable clock\n");
>> +return ret;
>> +}
>> +
>> +ret = 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

Hi Oleksij,

On 06/14/2017 03:48 PM, Oleksij Rempel wrote:

From: Oleksij Rempel 

this driver was tested on NXP imx7d but should work on
imx6sx as well.
It will upload firmware to OCRAM, which shared memory between
Cortex A7 and Cortex M4, then turn M4 on.


Mostly looks fine, need to address few comments. I take it that you 
haven't added the binding since this is just an RFC.




Signed-off-by: Oleksij Rempel 
---
  drivers/remoteproc/Kconfig |   8 ++
  drivers/remoteproc/Makefile|   1 +
  drivers/remoteproc/imx_rproc.c | 264 +
  3 files changed, 273 insertions(+)
  create mode 100644 drivers/remoteproc/imx_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..8d33bff4f530 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
tristate
depends on REMOTEPROC
  
+config IMX_REMOTEPROC

+   tristate "IMX6/7 remoteproc support"
+   depends on SOC_IMX6SX || SOC_IMX7D
+   depends on REMOTEPROC
+   help
+ TODO, write some thing here
+ This can be either built-in or a loadable module.
+
  endif # REMOTEPROC
  
  endmenu

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..d351c25cdb4e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,3 +19,4 @@ qcom_wcnss_pil-y  += qcom_wcnss.o
  qcom_wcnss_pil-y  += qcom_wcnss_iris.o
  obj-$(CONFIG_ST_REMOTEPROC)   += st_remoteproc.o
  obj-$(CONFIG_ST_SLIM_REMOTEPROC)  += st_slim_rproc.o
+obj-$(CONFIG_IMX_REMOTEPROC)   += imx_rproc.o
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
new file mode 100644
index ..6bef85237a27
--- /dev/null
+++ b/drivers/remoteproc/imx_rproc.c
@@ -0,0 +1,264 @@
+/*
+ * Oleksij Rempel 


Please add a blank line here.


+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX7D_ENABLE_M4BIT(3)
+#define IMX7D_SW_M4P_RST   BIT(2)
+#define IMX7D_SW_M4C_RST   BIT(1)
+#define IMX7D_SW_M4C_NON_SCLR_RST  BIT(0)
+
+#define IMX7D_M4_RST_MASK  0xf
+
+
+#define IMX7D_RPROC_MEM_MAX2
+enum {
+   IMX7D_RPROC_IMEM,
+   IMX7D_RPROC_DMEM,
+};
+
+static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
+   [IMX7D_RPROC_IMEM]  = "imem",
+   [IMX7D_RPROC_DMEM]  = "dmem",
+};


Do you really need these to be globally defined? You only need them in 
the addr_init function, they can be made local to that function.



+
+/**
+ * struct imx_rproc_mem - slim internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @size: Size of the memory region
+ */
+struct imx_rproc_mem {
+   void __iomem *cpu_addr;
+   phys_addr_t bus_addr;


Are the rproc-view of these regions same as the bus addresses or
do they have their own local addresses? If latter, images that are built 
for those will be using those addresses in which case you need some 
offset calculation in the da_to_va ops?



+   size_t size;
+};
+
+struct imx_rproc_dcfg {
+   int offset;
+};
+
+struct imx_rproc {
+   struct device   *dev;
+   struct regmap   *regmap;
+   struct rproc*rproc;
+   const struct imx_rproc_dcfg *dcfg;


No need of aligning the variables, you can get rid of the additional 
whitespaces and maintain consistent style.



+   struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
+   struct clk  *clk;
+};
+
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
+   .offset = 0xc,
+};
+
+static int imx_rproc_start(struct rproc *rproc)
+{
+   struct imx_rproc *priv = rproc->priv;
+   const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+   struct device *dev = priv->dev;
+   int ret;
+
+   ret = clk_enable(priv->clk);
+   if (ret) {
+   dev_err(>dev, "Failed to enable clock\n");
+   return ret;
+   }
+
+   ret = regmap_update_bits(priv->regmap, dcfg->offset,
+IMX7D_M4_RST_MASK,
+IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | 
IMX7D_ENABLE_M4);
+

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

Hi Oleksij,

On 06/14/2017 03:48 PM, Oleksij Rempel wrote:

From: Oleksij Rempel 

this driver was tested on NXP imx7d but should work on
imx6sx as well.
It will upload firmware to OCRAM, which shared memory between
Cortex A7 and Cortex M4, then turn M4 on.


Mostly looks fine, need to address few comments. I take it that you 
haven't added the binding since this is just an RFC.




Signed-off-by: Oleksij Rempel 
---
  drivers/remoteproc/Kconfig |   8 ++
  drivers/remoteproc/Makefile|   1 +
  drivers/remoteproc/imx_rproc.c | 264 +
  3 files changed, 273 insertions(+)
  create mode 100644 drivers/remoteproc/imx_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..8d33bff4f530 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
tristate
depends on REMOTEPROC
  
+config IMX_REMOTEPROC

+   tristate "IMX6/7 remoteproc support"
+   depends on SOC_IMX6SX || SOC_IMX7D
+   depends on REMOTEPROC
+   help
+ TODO, write some thing here
+ This can be either built-in or a loadable module.
+
  endif # REMOTEPROC
  
  endmenu

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..d351c25cdb4e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,3 +19,4 @@ qcom_wcnss_pil-y  += qcom_wcnss.o
  qcom_wcnss_pil-y  += qcom_wcnss_iris.o
  obj-$(CONFIG_ST_REMOTEPROC)   += st_remoteproc.o
  obj-$(CONFIG_ST_SLIM_REMOTEPROC)  += st_slim_rproc.o
+obj-$(CONFIG_IMX_REMOTEPROC)   += imx_rproc.o
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
new file mode 100644
index ..6bef85237a27
--- /dev/null
+++ b/drivers/remoteproc/imx_rproc.c
@@ -0,0 +1,264 @@
+/*
+ * Oleksij Rempel 


Please add a blank line here.


+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX7D_ENABLE_M4BIT(3)
+#define IMX7D_SW_M4P_RST   BIT(2)
+#define IMX7D_SW_M4C_RST   BIT(1)
+#define IMX7D_SW_M4C_NON_SCLR_RST  BIT(0)
+
+#define IMX7D_M4_RST_MASK  0xf
+
+
+#define IMX7D_RPROC_MEM_MAX2
+enum {
+   IMX7D_RPROC_IMEM,
+   IMX7D_RPROC_DMEM,
+};
+
+static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
+   [IMX7D_RPROC_IMEM]  = "imem",
+   [IMX7D_RPROC_DMEM]  = "dmem",
+};


Do you really need these to be globally defined? You only need them in 
the addr_init function, they can be made local to that function.



+
+/**
+ * struct imx_rproc_mem - slim internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @size: Size of the memory region
+ */
+struct imx_rproc_mem {
+   void __iomem *cpu_addr;
+   phys_addr_t bus_addr;


Are the rproc-view of these regions same as the bus addresses or
do they have their own local addresses? If latter, images that are built 
for those will be using those addresses in which case you need some 
offset calculation in the da_to_va ops?



+   size_t size;
+};
+
+struct imx_rproc_dcfg {
+   int offset;
+};
+
+struct imx_rproc {
+   struct device   *dev;
+   struct regmap   *regmap;
+   struct rproc*rproc;
+   const struct imx_rproc_dcfg *dcfg;


No need of aligning the variables, you can get rid of the additional 
whitespaces and maintain consistent style.



+   struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
+   struct clk  *clk;
+};
+
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
+   .offset = 0xc,
+};
+
+static int imx_rproc_start(struct rproc *rproc)
+{
+   struct imx_rproc *priv = rproc->priv;
+   const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+   struct device *dev = priv->dev;
+   int ret;
+
+   ret = clk_enable(priv->clk);
+   if (ret) {
+   dev_err(>dev, "Failed to enable clock\n");
+   return ret;
+   }
+
+   ret = regmap_update_bits(priv->regmap, dcfg->offset,
+IMX7D_M4_RST_MASK,
+IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | 
IMX7D_ENABLE_M4);
+   if (ret) {
+   dev_err(dev, "Filed to enable M4!\n");
+