[PATCH 1/2] remoteproc: imx_rproc: enlarge IMX7D_RPROC_MEM_MAX

[PATCH 1/2] remoteproc: imx_rproc: enlarge IMX7D_RPROC_MEM_MAX

[Peng Fan (OSS)]

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

8 is not enough when we need more, so enlarge IMX7D_RPROC_MEM_MAX to 32,
and also rename it to IMX_RPROC_MEM_MAX which make more sense.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 6d3207ccbaef..24275429a7cc 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -48,7 +48,7 @@
 					 | IMX6SX_SW_M4C_NON_SCLR_RST \
 					 | IMX6SX_SW_M4C_RST)
 
-#define IMX7D_RPROC_MEM_MAX		8
+#define IMX_RPROC_MEM_MAX		32
 
 /**
  * struct imx_rproc_mem - slim internal memory structure
@@ -88,7 +88,7 @@ struct imx_rproc {
 	struct regmap			*regmap;
 	struct rproc			*rproc;
 	const struct imx_rproc_dcfg	*dcfg;
-	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
+	struct imx_rproc_mem		mem[IMX_RPROC_MEM_MAX];
 	struct clk			*clk;
 	struct mbox_client		cl;
 	struct mbox_chan		*tx_ch;
@@ -272,7 +272,7 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *i
 	if (imx_rproc_da_to_sys(priv, da, len, &sys))
 		return NULL;
 
-	for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
+	for (i = 0; i < IMX_RPROC_MEM_MAX; i++) {
 		if (sys >= priv->mem[i].sys_addr && sys + len <
 		    priv->mem[i].sys_addr +  priv->mem[i].size) {
 			unsigned int offset = sys - priv->mem[i].sys_addr;
@@ -425,7 +425,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 		if (!(att->flags & ATT_OWN))
 			continue;
 
-		if (b >= IMX7D_RPROC_MEM_MAX)
+		if (b >= IMX_RPROC_MEM_MAX)
 			break;
 
 		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
@@ -459,7 +459,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 			return err;
 		}
 
-		if (b >= IMX7D_RPROC_MEM_MAX)
+		if (b >= IMX_RPROC_MEM_MAX)
 			break;
 
 		/* Not use resource version, because we might share region */
-- 
2.30.0

[PATCH 2/2] remoteproc: imx_rproc: support remote cores booted before Linux Kernel

[Peng Fan (OSS)]

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

Support remote cores booted before Linux Kernel booting.

Add rsc_table to hold the resource table published by remote cores
Add attach hook
Add imx_rproc_detect_mode to detect remote cores' working mode, and if
remote cores are booted before booting Linux Kernel, parse the memory
regions and initialize the table_ptr, table_sz, cached_table.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 24275429a7cc..fdaaf7599cc8 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -74,6 +74,16 @@ struct imx_rproc_att {
 	int flags;
 };
 
+enum imx_rproc_mode {
+	/* Linux load/kick remote core */
+	IMX_RPROC_NORMAL,
+	/*
+	 * remote core booted before kicking Linux, and remote core
+	 * could be stopped & restarted by Linux
+	 */
+	IMX_RPROC_EARLY_BOOT,
+};
+
 struct imx_rproc_dcfg {
 	u32				src_reg;
 	u32				src_mask;
@@ -95,6 +105,8 @@ struct imx_rproc {
 	struct mbox_chan		*rx_ch;
 	struct work_struct		rproc_work;
 	struct workqueue_struct		*workqueue;
+	enum imx_rproc_mode		mode;
+	void __iomem			*rsc_table;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
@@ -229,6 +241,9 @@ static int imx_rproc_stop(struct rproc *rproc)
 	if (ret)
 		dev_err(dev, "Failed to stop M4!\n");
 
+	if (priv->mode == IMX_RPROC_EARLY_BOOT)
+		priv->mode = IMX_RPROC_NORMAL;
+
 	return ret;
 }
 
@@ -398,9 +413,15 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
 			__func__, vqid, err);
 }
 
+static int imx_rproc_attach(struct rproc *rproc)
+{
+	return 0;
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
+	.attach		= imx_rproc_attach,
 	.kick		= imx_rproc_kick,
 	.da_to_va       = imx_rproc_da_to_va,
 	.load		= rproc_elf_load_segments,
@@ -470,6 +491,8 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 		}
 		priv->mem[b].sys_addr = res.start;
 		priv->mem[b].size = resource_size(&res);
+		if (!strcmp(node->name, "rsc_table"))
+			priv->rsc_table = priv->mem[b].cpu_addr;
 		b++;
 	}
 
@@ -536,6 +559,43 @@ static void imx_rproc_free_mbox(struct rproc *rproc)
 	mbox_free_channel(priv->rx_ch);
 }
 
+static int imx_rproc_detect_mode(struct imx_rproc *priv)
+{
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	struct rproc *rproc = priv->rproc;
+	struct device *dev = priv->dev;
+	int ret;
+	u32 val;
+
+	ret = regmap_read(priv->regmap, dcfg->src_reg, &val);
+	if (ret) {
+		dev_err(dev, "Failed to read src\n");
+		return ret;
+	}
+
+	if (!(val & dcfg->src_stop))
+		priv->mode = IMX_RPROC_EARLY_BOOT;
+	else
+		priv->mode = IMX_RPROC_NORMAL;
+
+	if (priv->mode == IMX_RPROC_EARLY_BOOT) {
+		priv->rproc->state = RPROC_DETACHED;
+
+		ret = imx_rproc_parse_memory_regions(priv->rproc);
+		if (ret)
+			return ret;
+
+		if (!priv->rsc_table)
+			return 0;
+
+		rproc->table_ptr = (struct resource_table *)priv->rsc_table;
+		rproc->table_sz = SZ_1K;
+		rproc->cached_table = NULL;
+	}
+
+	return 0;
+}
+
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -590,6 +650,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_mbox;
 	}
 
+	ret = imx_rproc_detect_mode(priv);
+	if (ret)
+		goto err_put_mbox;
+
 	priv->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to get clock\n");
-- 
2.30.0

Re: [PATCH 1/2] remoteproc: imx_rproc: enlarge IMX7D_RPROC_MEM_MAX

[Mathieu Poirier]

Hi Peng,

On Fri, Mar 19, 2021 at 06:47:07PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> 8 is not enough when we need more, so enlarge IMX7D_RPROC_MEM_MAX to 32,
> and also rename it to IMX_RPROC_MEM_MAX which make more sense.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 6d3207ccbaef..24275429a7cc 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -48,7 +48,7 @@
>  					 | IMX6SX_SW_M4C_NON_SCLR_RST \
>  					 | IMX6SX_SW_M4C_RST)
>  
> -#define IMX7D_RPROC_MEM_MAX		8
> +#define IMX_RPROC_MEM_MAX		32

The size of structure imx_rproc_att_imx7d and imx_rproc_att_imx6sx have
not changed nor has there been an addition of new imx_rproc_att that would
justify the change.

It seems to me you are working on something internally and this patch is in
preparation for that.  If that is the case then please resubmit this patch with
the rest of the code.

Thanks,
Mathieu 

>  
>  /**
>   * struct imx_rproc_mem - slim internal memory structure
> @@ -88,7 +88,7 @@ struct imx_rproc {
>  	struct regmap			*regmap;
>  	struct rproc			*rproc;
>  	const struct imx_rproc_dcfg	*dcfg;
> -	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
> +	struct imx_rproc_mem		mem[IMX_RPROC_MEM_MAX];
>  	struct clk			*clk;
>  	struct mbox_client		cl;
>  	struct mbox_chan		*tx_ch;
> @@ -272,7 +272,7 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *i
>  	if (imx_rproc_da_to_sys(priv, da, len, &sys))
>  		return NULL;
>  
> -	for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
> +	for (i = 0; i < IMX_RPROC_MEM_MAX; i++) {
>  		if (sys >= priv->mem[i].sys_addr && sys + len <
>  		    priv->mem[i].sys_addr +  priv->mem[i].size) {
>  			unsigned int offset = sys - priv->mem[i].sys_addr;
> @@ -425,7 +425,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  		if (!(att->flags & ATT_OWN))
>  			continue;
>  
> -		if (b >= IMX7D_RPROC_MEM_MAX)
> +		if (b >= IMX_RPROC_MEM_MAX)
>  			break;
>  
>  		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
> @@ -459,7 +459,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  			return err;
>  		}
>  
> -		if (b >= IMX7D_RPROC_MEM_MAX)
> +		if (b >= IMX_RPROC_MEM_MAX)
>  			break;
>  
>  		/* Not use resource version, because we might share region */
> -- 
> 2.30.0
> 

Re: [PATCH 1/2] remoteproc: imx_rproc: enlarge IMX7D_RPROC_MEM_MAX

[Mathieu Poirier]

On Thu, Mar 25, 2021 at 05:00:42PM -0600, Mathieu Poirier wrote:
> Hi Peng,
> 
> On Fri, Mar 19, 2021 at 06:47:07PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > 8 is not enough when we need more, so enlarge IMX7D_RPROC_MEM_MAX to 32,
> > and also rename it to IMX_RPROC_MEM_MAX which make more sense.
> > 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> > index 6d3207ccbaef..24275429a7cc 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -48,7 +48,7 @@
> >  					 | IMX6SX_SW_M4C_NON_SCLR_RST \
> >  					 | IMX6SX_SW_M4C_RST)
> >  
> > -#define IMX7D_RPROC_MEM_MAX		8
> > +#define IMX_RPROC_MEM_MAX		32
> 
> The size of structure imx_rproc_att_imx7d and imx_rproc_att_imx6sx have
> not changed nor has there been an addition of new imx_rproc_att that would
> justify the change.
> 
> It seems to me you are working on something internally and this patch is in
> preparation for that.  If that is the case then please resubmit this patch with
> the rest of the code.

Reviewing the other patch in this set [1], I understand why the extension is
needed.  Without a cover letter I assume the patches were not related.  Next
time please add one so that I know exactly what is going on.  Adding a line in
the changelog that explains why it is needed, i.e for the resource table, would
also be helpful.

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

[1]. remoteproc: imx_rproc: support remote cores booted before Linux Kernel

> 
> Thanks,
> Mathieu 
> 
> >  
> >  /**
> >   * struct imx_rproc_mem - slim internal memory structure
> > @@ -88,7 +88,7 @@ struct imx_rproc {
> >  	struct regmap			*regmap;
> >  	struct rproc			*rproc;
> >  	const struct imx_rproc_dcfg	*dcfg;
> > -	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
> > +	struct imx_rproc_mem		mem[IMX_RPROC_MEM_MAX];
> >  	struct clk			*clk;
> >  	struct mbox_client		cl;
> >  	struct mbox_chan		*tx_ch;
> > @@ -272,7 +272,7 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *i
> >  	if (imx_rproc_da_to_sys(priv, da, len, &sys))
> >  		return NULL;
> >  
> > -	for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
> > +	for (i = 0; i < IMX_RPROC_MEM_MAX; i++) {
> >  		if (sys >= priv->mem[i].sys_addr && sys + len <
> >  		    priv->mem[i].sys_addr +  priv->mem[i].size) {
> >  			unsigned int offset = sys - priv->mem[i].sys_addr;
> > @@ -425,7 +425,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
> >  		if (!(att->flags & ATT_OWN))
> >  			continue;
> >  
> > -		if (b >= IMX7D_RPROC_MEM_MAX)
> > +		if (b >= IMX_RPROC_MEM_MAX)
> >  			break;
> >  
> >  		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
> > @@ -459,7 +459,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
> >  			return err;
> >  		}
> >  
> > -		if (b >= IMX7D_RPROC_MEM_MAX)
> > +		if (b >= IMX_RPROC_MEM_MAX)
> >  			break;
> >  
> >  		/* Not use resource version, because we might share region */
> > -- 
> > 2.30.0
> > 

Re: [PATCH 2/2] remoteproc: imx_rproc: support remote cores booted before Linux Kernel

[Mathieu Poirier]

On Fri, Mar 19, 2021 at 06:47:08PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Support remote cores booted before Linux Kernel booting.
> 
> Add rsc_table to hold the resource table published by remote cores
> Add attach hook

Missing a period "." and a new line between the paragraphs.

> Add imx_rproc_detect_mode to detect remote cores' working mode, and if
> remote cores are booted before booting Linux Kernel, parse the memory
> regions and initialize the table_ptr, table_sz, cached_table.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 64 ++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 24275429a7cc..fdaaf7599cc8 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -74,6 +74,16 @@ struct imx_rproc_att {
>  	int flags;
>  };
>  
> +enum imx_rproc_mode {
> +	/* Linux load/kick remote core */
> +	IMX_RPROC_NORMAL,
> +	/*
> +	 * remote core booted before kicking Linux, and remote core
> +	 * could be stopped & restarted by Linux
> +	 */
> +	IMX_RPROC_EARLY_BOOT,
> +};
> +
>  struct imx_rproc_dcfg {
>  	u32				src_reg;
>  	u32				src_mask;
> @@ -95,6 +105,8 @@ struct imx_rproc {
>  	struct mbox_chan		*rx_ch;
>  	struct work_struct		rproc_work;
>  	struct workqueue_struct		*workqueue;
> +	enum imx_rproc_mode		mode;
> +	void __iomem			*rsc_table;
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
> @@ -229,6 +241,9 @@ static int imx_rproc_stop(struct rproc *rproc)
>  	if (ret)
>  		dev_err(dev, "Failed to stop M4!\n");
>  
> +	if (priv->mode == IMX_RPROC_EARLY_BOOT)
> +		priv->mode = IMX_RPROC_NORMAL;
> +

Why is this needed?  What scenario are you trying to address?

>  	return ret;
>  }
>  
> @@ -398,9 +413,15 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
>  			__func__, vqid, err);
>  }
>  
> +static int imx_rproc_attach(struct rproc *rproc)
> +{
> +	return 0;
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
> +	.attach		= imx_rproc_attach,
>  	.kick		= imx_rproc_kick,
>  	.da_to_va       = imx_rproc_da_to_va,
>  	.load		= rproc_elf_load_segments,
> @@ -470,6 +491,8 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  		}
>  		priv->mem[b].sys_addr = res.start;
>  		priv->mem[b].size = resource_size(&res);
> +		if (!strcmp(node->name, "rsc_table"))
> +			priv->rsc_table = priv->mem[b].cpu_addr;
>  		b++;
>  	}
>  
> @@ -536,6 +559,43 @@ static void imx_rproc_free_mbox(struct rproc *rproc)
>  	mbox_free_channel(priv->rx_ch);
>  }
>  
> +static int imx_rproc_detect_mode(struct imx_rproc *priv)
> +{
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> +	struct rproc *rproc = priv->rproc;
> +	struct device *dev = priv->dev;
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(priv->regmap, dcfg->src_reg, &val);
> +	if (ret) {
> +		dev_err(dev, "Failed to read src\n");
> +		return ret;
> +	}
> +
> +	if (!(val & dcfg->src_stop))
> +		priv->mode = IMX_RPROC_EARLY_BOOT;
> +	else
> +		priv->mode = IMX_RPROC_NORMAL;
> +
> +	if (priv->mode == IMX_RPROC_EARLY_BOOT) {
> +		priv->rproc->state = RPROC_DETACHED;
> +
> +		ret = imx_rproc_parse_memory_regions(priv->rproc);

If you do this here it won't be possible to reattach to the remote processor
once it has been detached.  This kind of memory parsing should go in
rproc_ops::prepare().  I suggest you look at what has been done for STM32, the
example there is quite good and simple.

> +		if (ret)
> +			return ret;
> +
> +		if (!priv->rsc_table)
> +			return 0;
> +
> +		rproc->table_ptr = (struct resource_table *)priv->rsc_table;

The core is taking care of that, see rproc_set_rsc_table() for details.  

> +		rproc->table_sz = SZ_1K;
> +		rproc->cached_table = NULL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int imx_rproc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -590,6 +650,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  		goto err_put_mbox;
>  	}
>  
> +	ret = imx_rproc_detect_mode(priv);
> +	if (ret)
> +		goto err_put_mbox;
> +

While reviewing this patch I noticed that of_node_put() is never called after
of_parse_phandle().  Please add a fix for that, in a patch on its own, in your
next revision.

Thanks,
Mathieu

>  	priv->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		dev_err(dev, "Failed to get clock\n");
> -- 
> 2.30.0
>